milleruntime commented on a change in pull request #2171:
URL: https://github.com/apache/accumulo/pull/2171#discussion_r657047459



##########
File path: 
server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java
##########
@@ -197,9 +197,11 @@ public boolean recoverLogs(KeyExtent extent, 
Collection<Collection<String>> walo
         synchronized (this) {
           if (!closeTasksQueued.contains(sortId) && 
!sortsQueued.contains(sortId)) {
             AccumuloConfiguration aconf = manager.getConfiguration();
+            @SuppressWarnings("deprecation")
             LogCloser closer = Property.createInstanceFromPropertyName(aconf,
-                Property.MANAGER_WALOG_CLOSER_IMPLEMETATION, LogCloser.class,
-                new HadoopLogCloser());
+                aconf.resolve(Property.MANAGER_WAL_CLOSER_IMPLEMENTATION,
+                    Property.MANAGER_WALOG_CLOSER_IMPLEMETATION),

Review comment:
       > having the intermediate version helps users easily transition their 
old config files with simple sed commands, and that will break if we don't have 
a direct mapping of all master. props to manager. ones.
   
   That is a good point. It is unfortunate that we will change a user's 
property to a deprecated one but having the intermediate property makes the 
changes more flexible. I think this is OK as long as the version that 
eventually drops the `walog` properties, does the replace in the upgrade code. 
We have become better at resolving things in the upgrade code but this is 
another example of why you should never skip Major or Minor versions when 
upgrading.




-- 
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to