ctubbsii commented on issue #708: Simplify API definition by moving impl packages from under API packages URL: https://github.com/apache/accumulo/issues/708#issuecomment-435994977 @jmark99 I think the following changes would be better: In `org.apache.accumulo.core`: | From | To | | ---- | -- | | client.impl | clientImpl | | client.impl.thrift | clientImpl.thrift | | client.lexicoder.impl | clientImpl.lexicoder | | client.mapred.impl | clientImpl.mapred | | client.mapreduce.impl | clientImpl.mapreduce | | client.mapreduce.lib.impl | clientImpl.mapreduce.lib | | data.impl | dataImpl | | data.thrift | dataImpl.thrift | In `org.apache.accumulo` (for minicluster): | From | To | | ---- | -- | | minicluster.impl | miniclusterImpl | For the security packages, I'm not sure, but have added inline questions: | From | To | Question | | ---- | -- | -------- | | security.crypto | crypto.security | What's the value in renaming this? At the very least, "security" here is redundant, since crypto implies a security feature. | | security.crypto.impl | crypto.security.impl | Not sure about this, but should be something like securityImpl.crypto? | | security.crypto.streams | crypto.security.streams | Not sure about this? | | security.thrift | impl.security.thrift | Should be securityImpl.thrift? | Changing from `x.impl` to `xImpl` keeps the implementation package sorted right next to the public API package `x`, which is convenient, but without polluting it with non-API sub-packages. Other things to note: changing the location of the thrift classes will require a change in our code generation scripts, which will also affect other modules.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
