ctubbsii commented on a change in pull request #2181:
URL: https://github.com/apache/accumulo/pull/2181#discussion_r665723147
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java
##########
@@ -35,11 +36,13 @@
import org.apache.accumulo.core.data.Mutation;
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.accumulo.server.fs.VolumeManager;
import org.apache.accumulo.server.fs.VolumeManagerImpl;
import org.apache.accumulo.start.spi.KeywordExecutable;
import org.apache.accumulo.tserver.log.DfsLogger;
import org.apache.accumulo.tserver.log.DfsLogger.LogHeaderIncompleteException;
-import org.apache.accumulo.tserver.log.RecoveryLogReader;
Review comment:
The subject line and ultimately the commit message when it is merged
will provide the narrative to frame the scope of the work that was done in the
commit. Currently, the subject line says "Update LogReader to utilize
RecoveryLogsIterator". If there were room for a longer description, it could
just as well have also said "... instead of RecoveryLogReader", because that's
what the code ended up doing. So, I would consider removing the unused code to
be part of this same change set and same scope of work. I don't see any reason
to defer to a future PR, because that would be like leaving commented out or
unused code in place. But, you gotta use your best judgment. Sometimes it's not
clear whether its better to do it as follow on or not. In this case, I think
removing the unused code should be included in the change.
--
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]
