keith-turner commented on code in PR #4207:
URL: https://github.com/apache/accumulo/pull/4207#discussion_r1480587564
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java:
##########
@@ -82,34 +86,32 @@ public class GarbageCollectWriteAheadLogs {
this.context = context;
this.fs = fs;
this.liveServers = liveServers;
- this.walMarker = new WalStateManager(context);
- this.store = () -> Iterators.concat(
- context.getAmple().readTablets().forLevel(DataLevel.ROOT).filter(new
HasWalsFilter())
- .fetch(LOCATION, LAST, LOGS, PREV_ROW, SUSPEND).build().iterator(),
-
context.getAmple().readTablets().forLevel(DataLevel.METADATA).filter(new
HasWalsFilter())
- .fetch(LOCATION, LAST, LOGS, PREV_ROW, SUSPEND).build().iterator(),
- context.getAmple().readTablets().forLevel(DataLevel.USER).filter(new
HasWalsFilter())
- .fetch(LOCATION, LAST, LOGS, PREV_ROW,
SUSPEND).build().iterator());
+ this.walMarker = getWalStateManager();
+ 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 liveTServerSet a started LiveTServerSet instance
- */
@VisibleForTesting
- GarbageCollectWriteAheadLogs(ServerContext context, VolumeManager fs,
- LiveTServerSet liveTServerSet, WalStateManager walMarker,
Iterable<TabletMetadata> store) {
- this.context = context;
- this.fs = fs;
- this.liveServers = liveTServerSet;
- this.walMarker = walMarker;
- this.store = store;
+ WalStateManager getWalStateManager() {
Review Comment:
The following is called in the constructor and the relies on the context
being set first. Think it may be best to pass it in to make the dependcy
clear. Also thinking the prefix create is better than get because the getter
seems like maybe it should return the instance var `walMarker`
```suggestion
WalStateManager createWalStateManager(ServerContext context) {
```
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java:
##########
@@ -82,34 +86,32 @@ public class GarbageCollectWriteAheadLogs {
this.context = context;
this.fs = fs;
this.liveServers = liveServers;
- this.walMarker = new WalStateManager(context);
- this.store = () -> Iterators.concat(
- context.getAmple().readTablets().forLevel(DataLevel.ROOT).filter(new
HasWalsFilter())
- .fetch(LOCATION, LAST, LOGS, PREV_ROW, SUSPEND).build().iterator(),
-
context.getAmple().readTablets().forLevel(DataLevel.METADATA).filter(new
HasWalsFilter())
- .fetch(LOCATION, LAST, LOGS, PREV_ROW, SUSPEND).build().iterator(),
- context.getAmple().readTablets().forLevel(DataLevel.USER).filter(new
HasWalsFilter())
- .fetch(LOCATION, LAST, LOGS, PREV_ROW,
SUSPEND).build().iterator());
+ this.walMarker = getWalStateManager();
+ 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 liveTServerSet a started LiveTServerSet instance
- */
@VisibleForTesting
- GarbageCollectWriteAheadLogs(ServerContext context, VolumeManager fs,
- LiveTServerSet liveTServerSet, WalStateManager walMarker,
Iterable<TabletMetadata> store) {
- this.context = context;
- this.fs = fs;
- this.liveServers = liveTServerSet;
- this.walMarker = walMarker;
- this.store = store;
+ WalStateManager getWalStateManager() {
+ return new WalStateManager(context);
+ }
+
+ @VisibleForTesting
+ Stream<TabletMetadata> getStore() {
Review Comment:
```suggestion
Stream<TabletMetadata> createStore() {
```
--
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]
