keith-turner commented on code in PR #4207:
URL: https://github.com/apache/accumulo/pull/4207#discussion_r1474620257
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java:
##########
@@ -83,13 +85,17 @@ public class GarbageCollectWriteAheadLogs {
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());
+ TabletsMetadata root =
context.getAmple().readTablets().forLevel(DataLevel.ROOT)
+ .filter(new HasWalsFilter()).fetch(LOCATION, LAST, LOGS, PREV_ROW,
SUSPEND).build();
+ TabletsMetadata metadata =
context.getAmple().readTablets().forLevel(DataLevel.METADATA)
+ .filter(new HasWalsFilter()).fetch(LOCATION, LAST, LOGS, PREV_ROW,
SUSPEND).build();
+ TabletsMetadata user =
context.getAmple().readTablets().forLevel(DataLevel.USER)
+ .filter(new HasWalsFilter()).fetch(LOCATION, LAST, LOGS, PREV_ROW,
SUSPEND).build();
+ this.store = Streams.concat(root.stream(), metadata.stream(),
user.stream()).onClose(() -> {
Review Comment:
I think the way this is structured the GC will only be able to run one cycle
and then will crash or not see data (not sure which) when attempting to run
subsequent cycles. The code uses this store to create an iterator over the
data each time the gc runs a cycle. So it should not be creating the streams
in the constructor, but instead it should create them each time on an as needed
basis. So maybe store could be changed to a `Supplier<Stream<TabletMetadata>>`.
I think the existing design of this code around this `store` is kinda
tricky. I was fighting with this same code in #4196 and made the same mistake
of creating stuff in the constructor until I caught it in testing. AFAICT this
design is driven by the desire for test to inject the `store`, was not really
sure how to improve it when working on #4196 because of the test. Would like
to figure how to make this code less misleading.
--
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]
