ctubbsii commented on code in PR #5863:
URL: https://github.com/apache/accumulo/pull/5863#discussion_r2331497316
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -1523,7 +1524,18 @@ public Optional<SplitComputations>
getSplitComputations() {
lastSplitComputation = new SoftReference<>(newComputation);
} catch (IOException e) {
lastSplitComputation.clear();
- log.error("Failed to compute split information from files " +
e.getMessage());
+ if (e.getClass().equals(FileNotFoundException.class)) {
+ Set<TabletFile> currentFiles = getDatafileManager().getFiles();
+ files.removeAll(currentFiles);
+ if (!files.isEmpty()) {
+ log.debug(
+ "Failed to compute split information. The following files have
most likely been garbage collected: {}",
+ files);
+ return Optional.empty();
+ }
Review Comment:
It might be easier to read this if it was in a `} catch
(FileNotFoundException e) {` block above the one for `IOException`. A few lines
would be duplicated, but I think it would be easier to follow than the `if`
block.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -1523,7 +1524,18 @@ public Optional<SplitComputations>
getSplitComputations() {
lastSplitComputation = new SoftReference<>(newComputation);
} catch (IOException e) {
lastSplitComputation.clear();
- log.error("Failed to compute split information from files " +
e.getMessage());
+ if (e.getClass().equals(FileNotFoundException.class)) {
+ Set<TabletFile> currentFiles = getDatafileManager().getFiles();
+ files.removeAll(currentFiles);
+ if (!files.isEmpty()) {
+ log.debug(
+ "Failed to compute split information. The following files have
most likely been garbage collected: {}",
+ files);
+ return Optional.empty();
+ }
+ }
+ log.error("Failed to compute split information from {} files in tablet
{}", files.size(),
+ getExtent(), e);
return Optional.empty();
Review Comment:
I tried to trace through to see why we're returning `Optional.empty()`, and
it looks like we eventually convert that to `null`, and then just ignore the
return type in a `void` method. There might be a few places where this matters,
but this is messy, and hard to look at. This logic to do a NOOP and expect the
split to just try again later, seems questionable and probably needs a refactor
at some point.
In the case where this code is called by `addSplits`, I think returning an
empty here will result in a special case of `NotServingTabletException` seen
and handled in the client code, which isn't necessarily going to retry... also
isn't necessarily correct either... since it was actually caused by a
compaction and a garbage collection cycle, not due to the tserver not hosting
the tablet.
Your fix is fine for the narrow case, but I question this logic more
generally. That said, splits work very differently in the main branch, so it's
probably not worth pursuing in 2.1.
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
