You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/12/13 07:28:44 UTC
[GitHub] [accumulo] ddanielr opened a new pull request, #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
ddanielr opened a new pull request, #3122:
URL: https://github.com/apache/accumulo/pull/3122
Fixes #3120 by adding constant for `"/users"` ZK path.
Also updated case style of `ZKUserPath` variable for consistency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
dlmarion merged PR #3122:
URL: https://github.com/apache/accumulo/pull/3122
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
ddanielr commented on PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#issuecomment-1348758931
@EdColeman yeah I wasn't sure what our desired state was as the formatter and style guide wasn't throwing errors and other private variables in the same classes also didn't match.
I'm happy to switch everything to the `zkUserPath` option instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1048866460
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -106,7 +106,7 @@ public static PermissionHandler getPermHandler(ServerContext context) {
protected SecurityOperation(ServerContext context, Authorizor author, Authenticator authent,
PermissionHandler pm) {
this.context = context;
- zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
+ zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + Constants.ZUSERS;
Review Comment:
Every time we use this constant, we are always appending it this way. This implies it might make more sense to just make a method dangle off of `context` that uses a memoized supplier:
```java
zkUserPath = context.zkUserPath();
```
```java
Supplier<String> zkUserPath = Suppliers.memoize(() -> Constants.ZROOT + "/" + getInstanceID() + Constants.ZUSERS);
// ...
public String zkUserPath() {
return zkUserPath.get();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1048996830
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -106,7 +106,7 @@ public static PermissionHandler getPermHandler(ServerContext context) {
protected SecurityOperation(ServerContext context, Authorizor author, Authenticator authent,
PermissionHandler pm) {
this.context = context;
- zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
+ zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + Constants.ZUSERS;
Review Comment:
I was just trying to suggest that 1) we reduce concatenation and 2) we localize the builders in a single place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1051105863
##########
server/base/src/main/java/org/apache/accumulo/server/ServerContext.java:
##########
@@ -113,6 +114,7 @@ private ServerContext(ServerInfo info) {
serverDirs = info.getServerDirs();
propStore = memoize(() -> ZooPropStore.initialize(getInstanceID(), getZooReaderWriter()));
+ zkUserPath = memoize(() -> Constants.ZROOT + "/" + getInstanceID() + Constants.ZUSERS);
Review Comment:
I don't think you need to use memoize here. That's for lazy loading instances with a supplier to load when needed but the zkUserPath is just a final String value so it should be fine to just use directly, ie `zkUserPath = Constants.ZROOT + "/" + getInstanceID() + Constants.ZUSERS;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1051106834
##########
server/base/src/main/java/org/apache/accumulo/server/ServerContext.java:
##########
@@ -113,6 +114,7 @@ private ServerContext(ServerInfo info) {
serverDirs = info.getServerDirs();
propStore = memoize(() -> ZooPropStore.initialize(getInstanceID(), getZooReaderWriter()));
+ zkUserPath = memoize(() -> Constants.ZROOT + "/" + getInstanceID() + Constants.ZUSERS);
Review Comment:
Oops just missed the early comments that suggested we SHOULD be using that so I guess ignore my comment :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#issuecomment-1348730428
I am surprised that the instance variable name convention that variables should start with a lower case letter is not complaining about ZKUserPath - with is seems to consistent with other variable names in places, but I thought that this would be against conventions.
In this case, I lean towards favoring convention and consistency could be examined separably (another pr making the other variables consistent and following convention - if that does not violate any API concerns)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1048990629
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -106,7 +106,7 @@ public static PermissionHandler getPermHandler(ServerContext context) {
protected SecurityOperation(ServerContext context, Authorizor author, Authenticator authent,
PermissionHandler pm) {
this.context = context;
- zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
+ zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + Constants.ZUSERS;
Review Comment:
Maybe as a follow-on? If I recall, almost always these path fragments are concatenated to build a path and the actual component name is not needed in isolation. The issue is that the instance id is a runtime value - but we could migrate to a set of static methods that accepted an instance id and returned a fully formed path.
The start usually is built with `ZooUtil.getRoot(iid)` and then a bunch of concatenations that may be easier to work with if it was along the lines of `ZooUtil.getZkUserPath(iid, userName)`
It may be preferable to commit this as it maintains the current convention and the use follow-on(s) to reduce the usage of Constant values everywhere. Or, maybe memorize then on context?
Not sure which approach would be best, just providing a suggestion for discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1048994404
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -106,7 +106,7 @@ public static PermissionHandler getPermHandler(ServerContext context) {
protected SecurityOperation(ServerContext context, Authorizor author, Authenticator authent,
PermissionHandler pm) {
this.context = context;
- zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
+ zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + Constants.ZUSERS;
Review Comment:
A good end goal would be to have our ZK connections chroot'd, so we don't have to prefix everything with the path to the instanceID. But, in order to achieve that goal, it would be easier if the paths were concatenated in fewer places. I proposed the improvement here, because I know it would serve that end goal, and it basically changes the same lines that are already being modified here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1049005037
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -106,7 +106,7 @@ public static PermissionHandler getPermHandler(ServerContext context) {
protected SecurityOperation(ServerContext context, Authorizor author, Authenticator authent,
PermissionHandler pm) {
this.context = context;
- zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
+ zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + Constants.ZUSERS;
Review Comment:
> I was just trying to suggest that 1) we reduce concatenation and 2) we localize the builders in a single place.
My suggestion achieves that, but just for this one path. The other constants or paths may not be as easy to do (may include more variables... like injection of table name, tserver name, etc.). This one is an easy one to do here, since every case is identical, the only variable component is the instance ID, and they all have a context to work with. So rather than focus on swapping out a constant, which is only a tiny, trivial improvement, we can swap out the string literal for a method, which is a slightly bigger improvement, and works towards another larger goal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
ddanielr commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1051100118
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -106,7 +106,7 @@ public static PermissionHandler getPermHandler(ServerContext context) {
protected SecurityOperation(ServerContext context, Authorizor author, Authenticator authent,
PermissionHandler pm) {
this.context = context;
- zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + "/users";
+ zkUserPath = Constants.ZROOT + "/" + context.getInstanceID() + Constants.ZUSERS;
Review Comment:
Added the context method for determining zkUserPath.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3122: Add Constant for Zookeeper user Path; Improve naming standardization.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3122:
URL: https://github.com/apache/accumulo/pull/3122#discussion_r1051494664
##########
server/base/src/main/java/org/apache/accumulo/server/ServerContext.java:
##########
@@ -113,6 +114,7 @@ private ServerContext(ServerInfo info) {
serverDirs = info.getServerDirs();
propStore = memoize(() -> ZooPropStore.initialize(getInstanceID(), getZooReaderWriter()));
+ zkUserPath = memoize(() -> Constants.ZROOT + "/" + getInstanceID() + Constants.ZUSERS);
Review Comment:
I recommended memoize because `getInstanceID()` may not be ready right away. We have had some bootstrapping issues with this, especially around the Initialize code. I could have been overly cautious in my recommendation, though. It might not actually be needed here. If it's not, I'm fine with dropping that bit and just making it a final String.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org