keith-turner commented on code in PR #5674:
URL: https://github.com/apache/accumulo/pull/5674#discussion_r2164707874
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java:
##########
@@ -119,43 +119,28 @@ Pair<Long,Map<TabletFile,DataFileValue>>
reserveFilesForScan() {
void returnFilesForScan(Long reservationId) {
- final Set<StoredTabletFile> filesToDelete = new HashSet<>();
-
- try {
- synchronized (tablet) {
- Set<StoredTabletFile> absFilePaths =
scanFileReservations.remove(reservationId);
+ synchronized (tablet) {
+ Set<StoredTabletFile> absFilePaths =
scanFileReservations.remove(reservationId);
- if (absFilePaths == null) {
- throw new IllegalArgumentException("Unknown scan reservation id " +
reservationId);
- }
+ if (absFilePaths == null) {
+ throw new IllegalArgumentException("Unknown scan reservation id " +
reservationId);
+ }
- boolean notify = false;
- try {
- for (StoredTabletFile path : absFilePaths) {
- long refCount = fileScanReferenceCounts.decrement(path, 1);
- if (refCount == 0) {
- if (filesToDeleteAfterScan.remove(path)) {
- filesToDelete.add(path);
- }
- notify = true;
- } else if (refCount < 0) {
- throw new IllegalStateException("Scan ref count for " + path + "
is " + refCount);
- }
- }
- } finally {
- if (notify) {
- tablet.notifyAll();
+ boolean notify = false;
+ try {
+ for (StoredTabletFile path : absFilePaths) {
+ long refCount = fileScanReferenceCounts.decrement(path, 1);
+ if (refCount == 0) {
+ filesToDeleteAfterScan.add(path);
Review Comment:
Do not need to add to `filesToDeleteAfterScan` here. This set
`filesToDeleteAfterScan` represents files that are in use by a scan and were
removed from the metadata table by a compaction. This method
`returnFilesForScan` is called when a scan completes, however the file used may
not have been removed from the tablet metadata and may still be active. Only
need to initially add files to `filesToDeleteAfterScan` in
`removeFilesAfterScan` which is called by the compaction code when it knows a
file its deleting from tablet metadata is also in use by a scan.
```suggestion
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -2220,8 +2220,22 @@ public MetadataUpdateCount getUpdateCount() {
return getDatafileManager().getUpdateCount();
}
- public void removeOrphanedScanRefs() {
- getDatafileManager().removeOrphanedScanRefs();
+ public void removeBatchedScanRefs() {
+ synchronized (this) {
+ if (isClosed() || isClosing()) {
+ return;
+ }
+ // TODO if a check is done here to see if there are orphaned scans and
none are found could
+ // return and not do the increment.
+ // this would cut down on the number of times the lock is acquired, but
not sure how clean
+ // this would be.
+ incrementWritesInProgress();
Review Comment:
For this TODO could add a method like the following to DataFileManager.
```java
boolean canScanFilesBeRemoved() {
synchronized (tablet) {
for(var path : filesToDeleteAfterScan) {
if (fileScanReferenceCounts.get(path) == 0){
return true;
}
}
return false;
}
}
```
```suggestion
if (!getDatafileManager().canScanFilesBeRemoved()){
return;
}
incrementWritesInProgress();
```
--
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
