ctubbsii commented on a change in pull request #2569:
URL: https://github.com/apache/accumulo/pull/2569#discussion_r828086151
##########
File path:
core/src/main/java/org/apache/accumulo/core/conf/ConfigurationTypeHelper.java
##########
@@ -181,7 +181,7 @@ public static double getFraction(String str) {
try {
instance = getClassInstance(context, clazzName, base);
} catch (RuntimeException | IOException | ReflectiveOperationException e) {
- log.warn("Failed to load class {}", clazzName, e);
+ log.warn("Failed to load context: {} class: {}", context, clazzName, e);
Review comment:
This log message reads like it failed to load the context rather than
the class.
```suggestion
log.warn("Failed to load class {} in classloader context {}",
clazzName, context, e);
```
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -107,6 +111,9 @@ private ServerContext(ServerInfo info) {
this.info = info;
zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
serverDirs = info.getServerDirs();
+
+ propStore = new ZooPropStore.Builder(info.getInstanceID(), zooReaderWriter,
+ zooReaderWriter.getSessionTimeout()).build();
Review comment:
I'm not sure a builder is needed at all. Could just do:
```suggestion
propStore = new ZooPropStore(this);
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java
##########
@@ -160,15 +160,17 @@ public long getUpdateCount() {
PrefixProps prefixProps = cachedPrefixProps.get(property);
- if (prefixProps == null || prefixProps.updateCount != getUpdateCount()) {
+ long currentCount = getUpdateCount();
+
+ if (prefixProps == null || prefixProps.updateCount != currentCount) {
prefixCacheUpdateLock.lock();
try {
// Very important that update count is read before getting properties.
Also only read it
// once.
- long updateCount = getUpdateCount();
+ long startCount = getUpdateCount();
Review comment:
I'm not sure this needed a name change. It's more to review, at a
minimum. It might be nice to go back through and try to minimize the changes
that aren't "germane" to this PR, like change in whitespace or variable renames.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
