You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ddanielr (via GitHub)" <gi...@apache.org> on 2023/10/03 19:47:51 UTC
[PR] Adds ReferenceScan type and updates GC [accumulo]
ddanielr opened a new pull request, #3805:
URL: https://github.com/apache/accumulo/pull/3805
This change adds a new subclass of Reference called ReferenceScan. This is so the Gc can determine if a candidate has been defeated by a file, dir, or scan reference.
If a candidate is considered to be inUse due to a scan reference, the Gc will not add it to the list of inUse candidates for possible deletion. This change closes 3804.
There also minor improvements to various comments and javadocs. I also added a warning statement to the experimental property description for `GC_REMOVE_IN_USE_CANDIDATES` to reference #3802.
I don't know if this is the best way to solve the issue mentioned in #3804, but it did accomplish it without changing the number of scan operations the GC has to make in a cycle.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370332232
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
Review Comment:
tableId lookup has been reduced to a single var. Stream names have been refactored to make their usage clearer.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370343280
##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java:
##########
@@ -1025,6 +1040,32 @@ public void testInUseDirReferenceCandidates() throws Exception {
assertTrue(gce.candidates.contains(candidate));
}
+ @Test
+ public void testInUseScanReferenceCandidates() throws Exception {
+ TestGCE gce = new TestGCE();
+
+ // InUse Scan Refs should not be removed.
+ var scanCandidate = gce.addCandidate("/4/t0/F010.rf");
+ var candOne = gce.addCandidate("/4/t0/F000.rf");
+ var candTwo = gce.addCandidate("/6/t0/F123.rf");
+ gce.addScanReference("4", null, "/t0/F010.rf");
+ gce.addFileReference("4", null, "/t0/F000.rf");
+
+ GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
+ gce.deleteInUseRefs = true;
+
+ gca.collect(gce);
+ assertRemoved(gce, candTwo);
+ assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne);
+ assertTrue(gce.candidates.contains(scanCandidate));
Review Comment:
Implemented this.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1663001715
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
+
// scans are normally empty, so only introduce a layer of indirection when needed
final var tmScans = tm.getScans();
if (!tmScans.isEmpty()) {
- fileStream = Stream.concat(fileStream, tmScans.stream());
+ var scanStream =
+ tmScans.stream().map(s -> new ReferenceScan(tm.getTableId(), s.getMetaUpdateDelete()));
Review Comment:
PR #4724 was created to close the loop
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1401297814
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
+
// scans are normally empty, so only introduce a layer of indirection when needed
final var tmScans = tm.getScans();
if (!tmScans.isEmpty()) {
- fileStream = Stream.concat(fileStream, tmScans.stream());
+ var scanStream =
+ tmScans.stream().map(s -> new ReferenceScan(tm.getTableId(), s.getMetaUpdateDelete()));
Review Comment:
@ddanielr To close the loop on this thread, did you end up creating a separate PR for this?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1366391923
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -894,7 +894,7 @@ public enum Property {
@Experimental
GC_REMOVE_IN_USE_CANDIDATES("gc.remove.in.use.candidates", "false", PropertyType.BOOLEAN,
"GC will remove deletion candidates that are in-use from the metadata location. "
- + "This is expected to increase the speed of subsequent GC runs",
+ + "This is expected to increase the speed of subsequent GC runs.",
Review Comment:
I'm not sure this dot matters. We already use sentences or sentence fragments for descriptions inconsistently. These descriptions are used to generate docs for the website for the properties, in a table. So, they can be sentence fragments, rather than full sentences. But consistency is useful too. So, maybe we should review all the descriptions and try to adhere to some kind of norm in the future? I'm not sure if it's worth it.
For this change, I say take it or leave it, it doesn't matter. I'm just thinking about the change in the context of all the properties.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -204,6 +204,8 @@ public static void replaceDatafiles(ServerContext context, KeyExtent extent,
tablet.putZooLock(zooLock);
tablet.mutate();
+ // Write candidates twice to avoid a possible race condition when removing InUse candidates
Review Comment:
The previous candidate writing could probably use a comment as well, to 1) link it to here, and 2) to specify how it is ordered relative to the tablet mutation that removes the file entries from the tablet.
##########
core/src/main/java/org/apache/accumulo/core/gc/ReferenceScan.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.gc;
+
+import org.apache.accumulo.core.data.TableId;
+
+/**
+ * A GC reference to a Scan entry.
+ */
+public class ReferenceScan extends ReferenceFile {
+
+ public ReferenceScan(TableId tableId, String metaDataEntry) {
+ super(tableId, metaDataEntry);
+ }
+
+ @Override
+ public boolean isScan() {
+ return true;
+ }
Review Comment:
I'm not really sure the method and the boolean really buy us much here. The truth value of the method is basically synonymous with `instanceof ReferenceScan`, so it seems a bit redundant. I would probably choose either one or the other. The subclass variant might also have issues with regards to the fact that it doesn't override equals and hashCode, which are defined in ReferenceFile. That could be a problem, because you can get compareTo returning 0, even though they are not equal. I'm sure there are other similar problems like this, too.
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
+
// scans are normally empty, so only introduce a layer of indirection when needed
final var tmScans = tm.getScans();
if (!tmScans.isEmpty()) {
- fileStream = Stream.concat(fileStream, tmScans.stream());
+ var scanStream =
+ tmScans.stream().map(s -> new ReferenceScan(tm.getTableId(), s.getMetaUpdateDelete()));
Review Comment:
Could just have a different constructor for these. Or, use static methods to get "named" constructors, as in:
```java
ReferenceFile fileRef = ReferenceFile.forRFile(tableId, f.getMetaUpdateDelete());
ReferenceFile scanRef = ReferenceFile.forScan(tableId, s.getMetaUpdateDelete());
```
That way you don't have to deal with the equals/hashCode issues, and can just use the isScan() method. Each of these static methods would call a private internal constructor:
```java
private ReferenceFile(TableId tableId, String metadataEntry, boolean isScan);
```
(can make the constructor package-private if you need it visible to be mocked for a unit test)
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
Review Comment:
Not sure if it matters, but could just get the tableId once for this loop.
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
Review Comment:
Also, renaming this variable to something like "fileRefStream" (or "fileStream", but renaming the current "fileStream"), will make the concatenation further down more clear. The generic name "stream" can get confusing if you're not deeply familiar with the code, especially since this variable is being reused as the concatenated stream as well.
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -183,7 +183,11 @@ private void removeCandidatesInUse(GarbageCollectionEnvironment gce,
GcCandidate gcTemp = candidateMap.remove(relativePath);
if (gcTemp != null) {
log.debug("File Candidate was still in use: {}", relativePath);
- inUseCandidates.add(gcTemp);
+ // Intentionally not adding scan candidates to inUseCandidates as they are only added
+ // once.
Review Comment:
Presumably, all the code we write is intentional, so I think that part of this comment isn't necessary. But more importantly, I think this comment isn't quite clear because it's not clear at this point in the code what the data structure "inUseCandidates" is expected to be used for. A variable name based on a full description would be something like: "candidatesForDeletionStillInUseAndCanBeDeletedBecauseANewCandidateWillBeCreatedLater".
Obviously, that's horrible. But, perhaps if the name were something like "candidateEntriesToBeDeleted", then the comment could be something like "Prevent deleting candidate entries that are still in-use only by scans, because they won't be recreated when the scan is finished." I think the key part here is trying to distinguish between deletion of candidate entries and the candidate files themselves.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#issuecomment-1762204883
Added the discussed fix for #3802 and removed the warning statement from the `GC_REMOVE_IN_USE_CANDIDATES` property.
fixes #3802
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #3805:
URL: https://github.com/apache/accumulo/pull/3805
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1368900604
##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java:
##########
@@ -1025,6 +1040,32 @@ public void testInUseDirReferenceCandidates() throws Exception {
assertTrue(gce.candidates.contains(candidate));
}
+ @Test
+ public void testInUseScanReferenceCandidates() throws Exception {
+ TestGCE gce = new TestGCE();
+
+ // InUse Scan Refs should not be removed.
+ var scanCandidate = gce.addCandidate("/4/t0/F010.rf");
+ var candOne = gce.addCandidate("/4/t0/F000.rf");
+ var candTwo = gce.addCandidate("/6/t0/F123.rf");
+ gce.addScanReference("4", null, "/t0/F010.rf");
+ gce.addFileReference("4", null, "/t0/F000.rf");
+
+ GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
+ gce.deleteInUseRefs = true;
+
+ gca.collect(gce);
+ assertRemoved(gce, candTwo);
+ assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne);
+ assertTrue(gce.candidates.contains(scanCandidate));
+
+ gce.removeScanReference("4", null, "/t0/F010.rf");
+ gca.collect(gce);
+ assertRemoved(gce, scanCandidate);
+ assertNoCandidatesRemoved(gce);
Review Comment:
When is the `scanCandidate` candidate removed and how does this call work with that removal?
##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java:
##########
@@ -1025,6 +1040,32 @@ public void testInUseDirReferenceCandidates() throws Exception {
assertTrue(gce.candidates.contains(candidate));
}
+ @Test
+ public void testInUseScanReferenceCandidates() throws Exception {
+ TestGCE gce = new TestGCE();
+
+ // InUse Scan Refs should not be removed.
+ var scanCandidate = gce.addCandidate("/4/t0/F010.rf");
+ var candOne = gce.addCandidate("/4/t0/F000.rf");
+ var candTwo = gce.addCandidate("/6/t0/F123.rf");
+ gce.addScanReference("4", null, "/t0/F010.rf");
+ gce.addFileReference("4", null, "/t0/F000.rf");
+
+ GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
+ gce.deleteInUseRefs = true;
+
+ gca.collect(gce);
+ assertRemoved(gce, candTwo);
+ assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne);
+ assertTrue(gce.candidates.contains(scanCandidate));
Review Comment:
Could be changed to the following more strict check?
```suggestion
assertEquals(Set.of(scanCandidates) ,gce.candidates);
```
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
+
// scans are normally empty, so only introduce a layer of indirection when needed
final var tmScans = tm.getScans();
if (!tmScans.isEmpty()) {
- fileStream = Stream.concat(fileStream, tmScans.stream());
+ var scanStream =
+ tmScans.stream().map(s -> new ReferenceScan(tm.getTableId(), s.getMetaUpdateDelete()));
Review Comment:
I like making RefrenceFile's constructor take a boolean isScan parameter instead of creating a new subclass. The static methods mentioned seem like a nice way to do that.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370342449
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -183,7 +183,11 @@ private void removeCandidatesInUse(GarbageCollectionEnvironment gce,
GcCandidate gcTemp = candidateMap.remove(relativePath);
if (gcTemp != null) {
log.debug("File Candidate was still in use: {}", relativePath);
- inUseCandidates.add(gcTemp);
+ // Intentionally not adding scan candidates to inUseCandidates as they are only added
+ // once.
Review Comment:
Changed the variable name from `inUseCandidates` to `candidateEntriesToBeDeleted` to help with readability.
Also updated the comment wording as well.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1367367631
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -894,7 +894,7 @@ public enum Property {
@Experimental
GC_REMOVE_IN_USE_CANDIDATES("gc.remove.in.use.candidates", "false", PropertyType.BOOLEAN,
"GC will remove deletion candidates that are in-use from the metadata location. "
- + "This is expected to increase the speed of subsequent GC runs",
+ + "This is expected to increase the speed of subsequent GC runs.",
Review Comment:
Looking at other properties, I agree that the comment descriptions are inconsistent.
I added some formatting checks and split that work off into #3877 to make all properties consistent.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370381681
##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java:
##########
@@ -1025,6 +1040,32 @@ public void testInUseDirReferenceCandidates() throws Exception {
assertTrue(gce.candidates.contains(candidate));
}
+ @Test
+ public void testInUseScanReferenceCandidates() throws Exception {
+ TestGCE gce = new TestGCE();
+
+ // InUse Scan Refs should not be removed.
+ var scanCandidate = gce.addCandidate("/4/t0/F010.rf");
+ var candOne = gce.addCandidate("/4/t0/F000.rf");
+ var candTwo = gce.addCandidate("/6/t0/F123.rf");
+ gce.addScanReference("4", null, "/t0/F010.rf");
+ gce.addFileReference("4", null, "/t0/F000.rf");
+
+ GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
+ gce.deleteInUseRefs = true;
+
+ gca.collect(gce);
+ assertRemoved(gce, candTwo);
+ assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne);
+ assertTrue(gce.candidates.contains(scanCandidate));
+
+ gce.removeScanReference("4", null, "/t0/F010.rf");
+ gca.collect(gce);
+ assertRemoved(gce, scanCandidate);
+ assertNoCandidatesRemoved(gce);
Review Comment:
Renamed `assertRemoved` to `assertFileDeleted` to better convey that the candidate has been processed by the GC and it's file has been "deleted" from hdfs.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370338791
##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -201,13 +202,16 @@ public Stream<Reference> getReferences() {
// combine all the entries read from file and scan columns in the metadata table
Stream<StoredTabletFile> fileStream = tm.getFiles().stream();
+ // map the files to Reference objects
+ var stream = fileStream.map(f -> new ReferenceFile(tm.getTableId(), f.getMetaUpdateDelete()));
+
// scans are normally empty, so only introduce a layer of indirection when needed
final var tmScans = tm.getScans();
if (!tmScans.isEmpty()) {
- fileStream = Stream.concat(fileStream, tmScans.stream());
+ var scanStream =
+ tmScans.stream().map(s -> new ReferenceScan(tm.getTableId(), s.getMetaUpdateDelete()));
Review Comment:
I like the static methods for "named" constructors.
I've gone ahead and implemented them for use instead of ReferenceScan.
It looks like we could also use them to replace the ReferenceDirectory class as well, but I'd want to handle that in a separate PR.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#issuecomment-1766437491
@ctubbsii Would it be possible to get a review of these changes? This PR has been sitting for a while and I'd like to close it out for 2.1.3
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1368893138
##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java:
##########
@@ -1025,6 +1040,32 @@ public void testInUseDirReferenceCandidates() throws Exception {
assertTrue(gce.candidates.contains(candidate));
}
+ @Test
+ public void testInUseScanReferenceCandidates() throws Exception {
+ TestGCE gce = new TestGCE();
+
+ // InUse Scan Refs should not be removed.
+ var scanCandidate = gce.addCandidate("/4/t0/F010.rf");
+ var candOne = gce.addCandidate("/4/t0/F000.rf");
+ var candTwo = gce.addCandidate("/6/t0/F123.rf");
+ gce.addScanReference("4", null, "/t0/F010.rf");
+ gce.addFileReference("4", null, "/t0/F000.rf");
+
+ GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
+ gce.deleteInUseRefs = true;
+
+ gca.collect(gce);
+ assertRemoved(gce, candTwo);
+ assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne);
+ assertTrue(gce.candidates.contains(scanCandidate));
Review Comment:
Could the following more strict check be done?
```suggestion
assertEquals(Set.of(scanCandidates) ,gce.candidates);
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370357248
##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java:
##########
@@ -1025,6 +1040,32 @@ public void testInUseDirReferenceCandidates() throws Exception {
assertTrue(gce.candidates.contains(candidate));
}
+ @Test
+ public void testInUseScanReferenceCandidates() throws Exception {
+ TestGCE gce = new TestGCE();
+
+ // InUse Scan Refs should not be removed.
+ var scanCandidate = gce.addCandidate("/4/t0/F010.rf");
+ var candOne = gce.addCandidate("/4/t0/F000.rf");
+ var candTwo = gce.addCandidate("/6/t0/F123.rf");
+ gce.addScanReference("4", null, "/t0/F010.rf");
+ gce.addFileReference("4", null, "/t0/F000.rf");
+
+ GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
+ gce.deleteInUseRefs = true;
+
+ gca.collect(gce);
+ assertRemoved(gce, candTwo);
+ assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne);
+ assertTrue(gce.candidates.contains(scanCandidate));
+
+ gce.removeScanReference("4", null, "/t0/F010.rf");
+ gca.collect(gce);
+ assertRemoved(gce, scanCandidate);
+ assertNoCandidatesRemoved(gce);
Review Comment:
The scanReference is removed as part of the GC run that is triggered by the collect method.
Since this test doesn't actually remove any files, we track what the GC would have done by where the candidate ends up after a GC run.
`assertRemoved` is the list of candidates that are successfully GC'd file paths.
`assertNoCandidatesRemoved` checks the candidate deletion list to ensure that the scanCandidate was not just removed from metadata without deleting it's referenced filepath.
I think these tests need to have better naming to convey the desired states.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370319709
##########
core/src/main/java/org/apache/accumulo/core/gc/ReferenceScan.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.gc;
+
+import org.apache.accumulo.core.data.TableId;
+
+/**
+ * A GC reference to a Scan entry.
+ */
+public class ReferenceScan extends ReferenceFile {
+
+ public ReferenceScan(TableId tableId, String metaDataEntry) {
+ super(tableId, metaDataEntry);
+ }
+
+ @Override
+ public boolean isScan() {
+ return true;
+ }
Review Comment:
Replacing the ReferenceScan class with static method constructors for ReferenceFile handles these concerns.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds ReferenceScan type and updates GC [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3805:
URL: https://github.com/apache/accumulo/pull/3805#discussion_r1370325175
##########
server/base/src/main/java/org/apache/accumulo/server/util/ManagerMetadataUtil.java:
##########
@@ -204,6 +204,8 @@ public static void replaceDatafiles(ServerContext context, KeyExtent extent,
tablet.putZooLock(zooLock);
tablet.mutate();
+ // Write candidates twice to avoid a possible race condition when removing InUse candidates
Review Comment:
Added an additional comment. Relative order is referenced in regards to the mutation.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org