kevinrr888 commented on code in PR #4790:
URL: https://github.com/apache/accumulo/pull/4790#discussion_r1717434847
##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java:
##########
@@ -287,13 +328,23 @@ public void replicationDelaysFileCollection() throws
Exception {
EasyMock.expectLastCall().once();
EasyMock.expect(mscanner.iterator()).andReturn(replicationWork.entrySet().iterator());
EasyMock.replay(context, fs, marker, tserverSet, rscanner, mscanner);
- GarbageCollectWriteAheadLogs gc = new
GarbageCollectWriteAheadLogs(context, fs, false,
- tserverSet, marker, tabletOnServer1List) {
- @Override
- protected Map<UUID,Path> getSortedWALogs() {
- return Collections.emptyMap();
- }
- };
+ GarbageCollectWriteAheadLogs gc =
+ new GarbageCollectWriteAheadLogs(context, fs, tserverSet, false) {
+ @Override
+ protected Map<UUID,Path> getSortedWALogs() {
+ return Collections.emptyMap();
+ }
+
+ @Override
+ WalStateManager createWalStateManager(ServerContext context) {
+ return marker;
+ }
+
+ @Override
+ Stream<TabletLocationState> createStore() {
+ return tabletOnServer2List;
Review Comment:
This one doesn't match the original gc object created with
`tabletOnServer1List`. Is this intentional?
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java:
##########
@@ -216,7 +217,7 @@ public void collect(GCStatus status) {
} finally {
span5.end();
}
-
+ hasCollected.set(true);
Review Comment:
It would probably be better to do this in a `finally` block for the
outermost `try`
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java:
##########
@@ -86,34 +89,32 @@ public class GarbageCollectWriteAheadLogs {
this.fs = fs;
this.useTrash = useTrash;
this.liveServers = liveServers;
- this.walMarker = new WalStateManager(context);
- this.store = () -> Iterators.concat(
- TabletStateStore.getStoreForLevel(DataLevel.ROOT, context).iterator(),
- TabletStateStore.getStoreForLevel(DataLevel.METADATA,
context).iterator(),
- TabletStateStore.getStoreForLevel(DataLevel.USER, context).iterator());
+ this.walMarker = createWalStateManager(context);
+ this.hasCollected = new AtomicBoolean(false);
}
- /**
- * Creates a new GC WAL object. Meant for testing -- allows mocked objects.
- *
- * @param context the collection server's context
- * @param fs volume manager to use
- * @param useTrash true to move files to trash rather than delete them
- * @param liveTServerSet a started LiveTServerSet instance
- */
@VisibleForTesting
- GarbageCollectWriteAheadLogs(ServerContext context, VolumeManager fs,
boolean useTrash,
- LiveTServerSet liveTServerSet, WalStateManager walMarker,
- Iterable<TabletLocationState> store) {
- this.context = context;
- this.fs = fs;
- this.useTrash = useTrash;
- this.liveServers = liveTServerSet;
- this.walMarker = walMarker;
- this.store = store;
+ WalStateManager createWalStateManager(ServerContext context) {
Review Comment:
I may be misunderstanding but what is the point of these new
`createWalStateManager` and `createStore` methods? In the testing code, I don't
see what the benefit of overriding these is. For example, the
`createWalStateManager` is overridden to return marker. How is this any
different from just simply passing marker to the constructor? Is the point to
just get rid of the one `GarbageCollectWriteAheadLogs` constructor that is
meant to be used for testing?
This could very well be the right thing to do, I just was a bit confused
looking at it
--
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]
