Add Constant for Zookeeper user Path; Improve naming standardization. #3122
Conversation
Creates a constant to replace the hard-coded "/users" string in ZK user operations. Standardizes pathing across all ZK classes that utilize a userPath.
Updates instances of the ZKUserPath variable case to match majority for ease of searching.
|
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) |
|
@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 |
Update all instances of the ZKUserPath with zkUserPath to conform to naming conventions.
Implements a ServerContext Method for determining zkUserPath instead of creating a local concatenated zkUserPath variable each time.
| serverDirs = info.getServerDirs(); | ||
|
|
||
| propStore = memoize(() -> ZooPropStore.initialize(getInstanceID(), getZooReaderWriter())); | ||
| zkUserPath = memoize(() -> Constants.ZROOT + "/" + getInstanceID() + Constants.ZUSERS); |
There was a problem hiding this 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;
There was a problem hiding this comment.
Oops just missed the early comments that suggested we SHOULD be using that so I guess ignore my comment :)
There was a problem hiding this 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.
Update the MockServerContext to expect a call to the zkUserPath() method.
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
Looks good. Assuming the build passes.
By the way, since you're a new contributor to this project, I just want to share my recommended howto guide on writing good commit messages: https://cbea.ms/git-commit/
Regardless of what you write, an existing committer should be checking and polishing up the commit message for accuracy, phrasing, etc. when we squash and merge, but it helps if they are written well up front. The biggest problem is long first lines, or omission of the blank second line. So, those are things to watch out for.
5 participants
Fixes #3120 by adding constant for
"/users"ZK path.Also updated case style of
ZKUserPathvariable for consistency.