GEODE-4633: un-deprecate peer-auth-init and more javadoc #1442
Conversation
jinmeiliao
requested review from
jdeppe-pivotal,
sboorlagadda,
PurelyApplied and
pdxrunner
| @@ -92,8 +92,8 @@ Properties getCredentials(Properties securityProps, DistributedMember server, bo | |||
| * @param securityProps | |||
| * @return the credentials to be used. It needs to contain "security-username" and | |||
| * "security-password" | |||
| * @deprecated As of Geode 1.3, please implement getCredentials(Properties, DistributedMember, | |||
| * boolean) | |||
| * @deprecated in Geode 1.3. never called by the product. use getCredentials(Properties | |||
There was a problem hiding this comment.
I don't know if the docs side of things differs, but I think two spaces after a period is still the standard for monospaced fonts. Capitalize after the periods in any case.
There was a problem hiding this comment.
Also, linking the javadoc would be nice.
* @deprecated in Geode 1.3. Never called by the product.
* Use {@link #getCredentials(Properties, DistributedMember, boolean)}
| * use log4j instead of these loggers. | ||
| * | ||
| * @since Geode 1.0. | ||
| * @deprecated in Geode 1.5, never called by the product, use init(LogWriter systemLogger, |
There was a problem hiding this comment.
Linking init could be nice. I'd prefer full sentences in the javadocs personally, rather that notes.
|
|
||
| @Override | ||
| public boolean authorize(Object principal, ResourcePermission permission) { | ||
| // only server-1 and server-2 are authenticated, but only server-1 is authorized |
There was a problem hiding this comment.
Nitpick: I'd drop the first "only."
| @Override | ||
| public boolean authorize(Object principal, ResourcePermission permission) { | ||
| // only server-1 and server-2 are authenticated, but only server-1 is authorized | ||
| if ("server-1".equals(principal)) { |
There was a problem hiding this comment.
Does it still read as well when this is simplified to return "server-1".equals(principal);
| @@ -54,8 +54,8 @@ default void init(LogWriter systemLogger, LogWriter securityLogger) | |||
| /** | |||
| * | |||
| * @since Geode 1.0. | |||
| * @deprecated in Geode 1.5, never called by the product, use init(LogWriter systemLogger, | |||
| * LogWriter securityLogger) | |||
| * @deprecated in Geode 1.5. Never called by the product. Use {link #init(LogWriter systemLogger, | |||
There was a problem hiding this comment.
Forgot the @ in @link.
If you're using IntelliJ, I think the default "view javadoc" hotkey is Ctrl-J, if you want to make sure linkage is working correctly.
| @@ -92,8 +92,8 @@ Properties getCredentials(Properties securityProps, DistributedMember server, bo | |||
| * @param securityProps | |||
| * @return the credentials to be used. It needs to contain "security-username" and | |||
| * "security-password" | |||
| * @deprecated in Geode 1.3. never called by the product. use getCredentials(Properties | |||
| * securityProps, DistributedMember server, boolean isPeer) | |||
| * @deprecated in Geode 1.3. Never called by the product. Use {link #getCredentials(Properties | |||
|
precheckin green |
|
precheckin green |
| /** | ||
| * Initialize the callback for a client/peer. This is invoked when a new connection from a | ||
| * client/peer is created with the host. | ||
| * | ||
| * Do not use these loggers, use log4j logger directly | ||
| * |
There was a problem hiding this comment.
I find the wording in the javadoc confusing - we are saying use the init(systemLogger, securityLogger) method, but don't use the logger arguments in the AuthInitialize implementors.
I understand that we need to continue support any existing implementations, so maybe the javadoc should indicate that new implementations should use log4j logger instead of the logger parameters.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.