ctubbsii commented on code in PR #2792:
URL: https://github.com/apache/accumulo/pull/2792#discussion_r972251517
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionEnvironment.java:
##########
@@ -72,6 +72,17 @@ public interface GarbageCollectionEnvironment {
*/
Stream<Reference> getReferences();
+ /**
+ * Return a list of all TableIDs in the
Review Comment:
It doesn't return a list. It returns a set.
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionEnvironment.java:
##########
@@ -72,6 +72,17 @@ public interface GarbageCollectionEnvironment {
*/
Stream<Reference> getReferences();
+ /**
+ * Return a list of all TableIDs in the
+ * {@link org.apache.accumulo.core.metadata.schema.Ample.DataLevel} for
which we are considering
+ * deletes. When operating on DataLevel.USER this will return all user table
ids. When operating
+ * on DataLevel.METADATA this will return the table id for the
accumulo.metadata table. When
+ * operating on DataLevel.ROOT this will return the table id for the
accumulo.root table.
+ *
+ * @return The table ids
+ */
+ Set<TableId> getCandidateTableIDs();
Review Comment:
The term "operating on" used throughout this javadoc is very confusing. What
does it mean to be "operating on" a particular level? I think we could choose a
better verb to understand what's going on. Maybe "when gathering file
candidates for deletion for DataLevel.USER, this will return a list of user
table IDs".
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -216,6 +225,46 @@ private long
removeBlipCandidates(GarbageCollectionEnvironment gce,
return blipCount;
}
+ @VisibleForTesting
+ /**
+ * Double check no tables were missed during GC
+ */
+ protected void ensureAllTablesChecked(Set<TableId> tableIdsBefore,
Set<TableId> tableIdsSeen,
+ Set<TableId> tableIdsAfter) {
+
+ // if a table was added or deleted during this run, it is acceptable to not
+ // have seen those tables ids when scanning the metadata table. So get the
intersection
+ final Set<TableId> tableIdsMustHaveSeen = new HashSet<>(tableIdsBefore);
+ tableIdsMustHaveSeen.retainAll(tableIdsAfter);
+
+ if (tableIdsMustHaveSeen.isEmpty() && !tableIdsSeen.isEmpty()) {
+ throw new RuntimeException("Garbage collection will not proceed because "
+ + "table ids were seen in the metadata table and none were seen
Zookeeper. "
+ + "This can have two causes. First, total number of tables going
to/from "
+ + "zero during a GC cycle will cause this. Second, it could be
caused by "
+ + "corruption of the metadata table and/or Zookeeper. Only the
second cause "
+ + "is problematic, but there is no way to distinguish between the
two causes "
+ + "so this GC cycle will not proceed. The first cause should be
transient "
+ + "and one would not expect to see this message repeated in
subsequent GC cycles.");
+ }
+
+ // From that intersection, remove all the table ids that were seen.
+ tableIdsMustHaveSeen.removeAll(tableIdsSeen);
+
+ // If anything is left then we missed a table and may not have removed
rfiles references
+ // from the candidates list that are acutally still in use, which would
+ // result in the rfiles being deleted in the next step of the GC process
+ if (!tableIdsMustHaveSeen.isEmpty()) {
+ log.error("TableIDs before: " + tableIdsBefore);
+ log.error("TableIDs after : " + tableIdsAfter);
+ log.error("TableIDs seen : " + tableIdsSeen);
+ log.error("TableIDs that should have been seen but were not: " +
tableIdsMustHaveSeen);
+ // maybe a scan failed?
+ throw new RuntimeException(
+ "Saw table IDs in ZK that were not in metadata table: " +
tableIdsMustHaveSeen);
+ }
Review Comment:
Please don't introduce new code with generic RTE being thrown. A more
specific RTE is always available and likely more appropriate.
Also, this situation is likely to be common if creating and deleting a lot
of tables. The canonical determination of whether a table exists or not is that
it has an entry in ZK... this is created before metadata entries, and is the
last thing removed when a table is deleted. So, it's quite normal and routine
for a table ID not to exist in the metadata table if it was just created or
deleted. This is not necessarily a problem and shouldn't throw an exception...
or we're going to see this a lot. We can probably start over gathering
candidates... but we shouldn't just crash.
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -462,4 +479,20 @@ public long getErrorsStat() {
public long getCandidatesStat() {
return candidates;
}
+
+ @Override
+ public Set<TableId> getCandidateTableIDs() {
+ if (level == DataLevel.ROOT) {
+ return Set.of(RootTable.ID);
+ } else if (level == DataLevel.METADATA) {
+ return Collections.singleton(MetadataTable.ID);
+ } else if (level == DataLevel.USER) {
+ Set<TableId> tableIds = new HashSet<>(getTableIDs());
+ tableIds.remove(MetadataTable.ID);
+ tableIds.remove(RootTable.ID);
+ return tableIds;
+ } else {
+ throw new RuntimeException("Unexpected Table in GC Env: " +
this.level.name());
Review Comment:
Choose more specific RTE
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -135,10 +137,14 @@ private SortedMap<String,String>
makeRelative(Collection<String> candidates) {
private void removeCandidatesInUse(GarbageCollectionEnvironment gce,
SortedMap<String,String> candidateMap) {
+
+ Set<TableId> tableIdsBefore = gce.getCandidateTableIDs();
+ Set<TableId> tableIdsSeen = new HashSet<>();
var refStream = gce.getReferences();
Iterator<Reference> iter = refStream.iterator();
while (iter.hasNext()) {
Reference ref = iter.next();
+ tableIdsSeen.add(ref.getTableId());
Review Comment:
This `var` obscures the type and variable name `refStream` implies it's a
Stream. But, then we call `.iterator()` on it. If it's actually a Stream, then
there are better ways to manipulate it than doing `while (iter.hasNext()) {
iter.next(); }`.
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -172,7 +177,19 @@ public Stream<Reference> getReferences() {
@Override
public Set<TableId> getTableIDs() {
- return context.getTableIdToNameMap().keySet();
+ final String tablesPath = context.getZooKeeperRoot() + Constants.ZTABLES;
+ final ZooReader zr = context.getZooReader();
+ final Set<TableId> tids = new HashSet<>();
+ while (true) {
+ try {
+ zr.sync(tablesPath);
+ zr.getChildren(tablesPath).forEach(t -> tids.add(TableId.of(t)));
+ return tids;
+ } catch (KeeperException | InterruptedException e) {
+ log.error("Error getting tables from ZooKeeper, retrying", e);
+ UtilWaitThread.sleepUninterruptibly(1, TimeUnit.SECONDS);
+ }
Review Comment:
First, this shouldn't loop if it's explicitly interrupted. The process
should probably die in that case. Second, if this does loop because of a
KeeperException, the set of table IDs that was constructed prior to the `while
(true)` loop could include deleted tables. Is that okay/intended? If so, a
small inline comment explaining why that's okay might be useful for future
onlookers.
--
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]
