keith-turner commented on code in PR #3150:
URL: https://github.com/apache/accumulo/pull/3150#discussion_r1064281018
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -489,7 +499,7 @@ public void close() {
}
private Map<KeyExtent,TabletMetadata>
reserveFilesInner(Collection<KeyExtent> extents,
Review Comment:
```suggestion
/*
* All extents passed in should end in either the returned map or the
failures set, but no extent should be in both.
*/
private Map<KeyExtent,TabletMetadata>
reserveFilesInner(Collection<KeyExtent> extents,
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -881,7 +903,7 @@ public InitialMultiScan startMultiScan(TInfo tinfo,
TCredentials credentials,
batch.put(extent, entry.getValue());
}
- try (ScanReservation reservation = reserveFiles(batch.keySet())) {
+ try (ScanReservation reservation = reserveFiles(batch)) {
HashMap<KeyExtent,TabletBase> tablets = new HashMap<>();
batch.keySet().forEach(extent -> {
Review Comment:
The following should be the set of non failure tablets.
```suggestion
reservation.tabletMetadata.keySet().forEach(extent -> {
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -633,17 +644,24 @@ private Map<KeyExtent,TabletMetadata>
reserveFilesInner(Collection<KeyExtent> ex
}
}
- protected ScanReservation reserveFiles(Collection<KeyExtent> extents)
- throws NotServingTabletException, AccumuloException {
+ protected ScanReservation reserveFiles(Map<KeyExtent,List<TRange>> extents)
+ throws AccumuloException {
long myReservationId = nextScanReservationId.incrementAndGet();
- Map<KeyExtent,TabletMetadata> tabletsMetadata = reserveFilesInner(extents,
myReservationId);
+ Set<KeyExtent> failedReservations = new HashSet<>();
+ Map<KeyExtent,TabletMetadata> tabletsMetadata =
+ reserveFilesInner(extents.keySet(), myReservationId,
failedReservations);
while (tabletsMetadata == null) {
- tabletsMetadata = reserveFilesInner(extents, myReservationId);
+ tabletsMetadata = reserveFilesInner(extents.keySet(), myReservationId,
failedReservations);
}
+ // Convert failures
Review Comment:
We could add a validation on the return.
```suggestion
if(!Collections.disjoint(tabletsMetadata.keySet(), failures) ||
!extents.keySet().equals(Sets.union(tabletsMetadata.keySet(), failures)) {
throw new AssertionError("bug in reserverFilesInner
"+extents.keySet()+" "+tabletsMetadata.keySet()+" "+failures);
}
// Convert failures
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -896,6 +918,10 @@ public InitialMultiScan startMultiScan(TInfo tinfo,
TCredentials credentials,
ssio, authorizations, waitForWrites, tSamplerConfig, batchTimeOut,
contextArg,
executionHints, getBatchScanTabletResolver(tablets), busyTimeout);
+ if (!reservation.getFailures().isEmpty()) {
+ ims.result.setFailures(reservation.getFailures());
+ }
+
Review Comment:
I was wondering if we need to handle the case of the delegate having already
set some failures. Digging around to answer that question I found [this
code](https://github.com/apache/accumulo/blob/af6b4978d42e7a1f9102a7ec9605be51c67bb595/server/tserver/src/main/java/org/apache/accumulo/tserver/scan/LookupTask.java#L108-L113).
Looking at that I think as long the tablet resolver does not return anything
that failed then it will naturally be added to the failure by
delegate.startMultiScan(). I added comments elsewhere about getting the tablet
resolver to not contain failures.
```suggestion
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -500,13 +510,13 @@ private Map<KeyExtent,TabletMetadata>
reserveFilesInner(Collection<KeyExtent> ex
var tabletMetadata = tabletsMetadata.get(extent);
if (tabletMetadata == null) {
LOG.info("RFFS {} extent not found in metadata table {}",
myReservationId, extent);
- throw new NotServingTabletException(extent.toThrift());
+ failures.add(extent);
}
if (!AssignmentHandler.checkTabletMetadata(extent, null, tabletMetadata,
true)) {
LOG.info("RFFS {} extent unable to load {} as AssignmentHandler
returned false",
myReservationId, extent);
- throw new NotServingTabletException(extent.toThrift());
+ failures.add(extent);
Review Comment:
```suggestion
failures.add(extent);
tabletsMetadata.remove(extent);
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -633,17 +644,24 @@ private Map<KeyExtent,TabletMetadata>
reserveFilesInner(Collection<KeyExtent> ex
}
}
- protected ScanReservation reserveFiles(Collection<KeyExtent> extents)
- throws NotServingTabletException, AccumuloException {
+ protected ScanReservation reserveFiles(Map<KeyExtent,List<TRange>> extents)
+ throws AccumuloException {
long myReservationId = nextScanReservationId.incrementAndGet();
- Map<KeyExtent,TabletMetadata> tabletsMetadata = reserveFilesInner(extents,
myReservationId);
+ Set<KeyExtent> failedReservations = new HashSet<>();
+ Map<KeyExtent,TabletMetadata> tabletsMetadata =
+ reserveFilesInner(extents.keySet(), myReservationId,
failedReservations);
while (tabletsMetadata == null) {
- tabletsMetadata = reserveFilesInner(extents, myReservationId);
+ tabletsMetadata = reserveFilesInner(extents.keySet(), myReservationId,
failedReservations);
Review Comment:
```suggestion
failedReservations.clear();
tabletsMetadata = reserveFilesInner(extents.keySet(), myReservationId,
failedReservations);
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -592,11 +602,12 @@ private Map<KeyExtent,TabletMetadata>
reserveFilesInner(Collection<KeyExtent> ex
getContext().getAmple().deleteScanServerFileReferences(refs);
LOG.info("RFFS {} extent unable to load {} as metadata no longer
referencing files",
myReservationId, extent);
- throw new NotServingTabletException(extent.toThrift());
+ failures.add(extent);
Review Comment:
```suggestion
failures.add(extent);
tabletsMetadata.remove(extent);
```
--
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]
