reschke commented on code in PR #993:
URL: https://github.com/apache/jackrabbit-oak/pull/993#discussion_r1285757504
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreHelper.java:
##########
@@ -72,8 +72,8 @@ public static void garbageReport(DocumentNodeStore dns) {
}
public static VersionGarbageCollector createVersionGC(
- DocumentNodeStore nodeStore, VersionGCSupport gcSupport) {
- return new VersionGarbageCollector(nodeStore, gcSupport);
+ DocumentNodeStore nodeStore, VersionGCSupport gcSupport, final
boolean detailedGCEnabled) {
Review Comment:
```suggestion
DocumentNodeStore nodeStore, VersionGCSupport gcSupport, boolean
detailedGCEnabled) {
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java:
##########
@@ -73,35 +89,57 @@ public class VersionGCRecommendations {
* @param vgc VersionGC support class
* @param options options for running the gc
* @param gcMonitor monitor class for messages
+ * @param detailedGCEnabled whether detailedGC is enabled or not
*/
public VersionGCRecommendations(long maxRevisionAgeMs, Checkpoints
checkpoints, Clock clock, VersionGCSupport vgc,
- VersionGCOptions options, GCMonitor gcMonitor) {
+ VersionGCOptions options, GCMonitor
gcMonitor, final boolean detailedGCEnabled) {
Review Comment:
```suggestion
VersionGCOptions options, GCMonitor
gcMonitor, boolean detailedGCEnabled) {
```
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreHelper.java:
##########
@@ -72,8 +72,8 @@ public static void garbageReport(DocumentNodeStore dns) {
}
public static VersionGarbageCollector createVersionGC(
- DocumentNodeStore nodeStore, VersionGCSupport gcSupport) {
- return new VersionGarbageCollector(nodeStore, gcSupport);
+ DocumentNodeStore nodeStore, VersionGCSupport gcSupport, final
boolean detailedGCEnabled) {
Review Comment:
Unneeded and not consistent with the code base...
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java:
##########
@@ -1669,6 +1670,24 @@ SortedMap<Revision, String> getLocalMap(String key) {
return map;
}
+ /**
+ * Returns name of all the properties on this document
+ * <p>
+ * Note: property names returned are escaped
+ * <p/>
+ * @return Set of all property names (escaped)
Review Comment:
Question: what kind of escaping is this?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCSupport.java:
##########
@@ -59,28 +69,61 @@ public VersionGCSupport(DocumentStore store) {
* @param toModified the upper bound modified timestamp (exclusive)
* @return matching documents.
*/
- public Iterable<NodeDocument> getPossiblyDeletedDocs(final long
fromModified,
- final long
toModified) {
- return filter(getSelectedDocuments(store, NodeDocument.DELETED_ONCE,
1), new Predicate<NodeDocument>() {
- @Override
- public boolean apply(NodeDocument input) {
- return input.wasDeletedOnce()
- && modifiedGreaterThanEquals(input, fromModified)
- && modifiedLessThan(input, toModified);
- }
+ public Iterable<NodeDocument> getPossiblyDeletedDocs(final long
fromModified, final long toModified) {
+ return stream(getSelectedDocuments(store, NodeDocument.DELETED_ONCE,
1).spliterator(), false)
+ .filter(input -> input.wasDeletedOnce() &&
modifiedGreaterThanEquals(input, fromModified) && modifiedLessThan(input,
toModified))
+ .collect(toList());
+ }
- private boolean modifiedGreaterThanEquals(NodeDocument doc,
- long time) {
- Long modified = doc.getModified();
- return modified != null &&
modified.compareTo(getModifiedInSecs(time)) >= 0;
- }
+ /**
+ * Returns documents that have a {@link NodeDocument#MODIFIED_IN_SECS}
value
+ * within the given range and are greater than given @{@link
NodeDocument#ID}.
+ * <p>
+ * The two passed modified timestamps are in milliseconds
+ * since the epoch and the implementation will convert them to seconds at
+ * the granularity of the {@link NodeDocument#MODIFIED_IN_SECS} field and
+ * then perform the comparison.
+ * <p/>
Review Comment:
```suggestion
*
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java:
##########
@@ -1669,6 +1670,24 @@ SortedMap<Revision, String> getLocalMap(String key) {
return map;
}
+ /**
+ * Returns name of all the properties on this document
+ * <p>
+ * Note: property names returned are escaped
+ * <p/>
Review Comment:
```suggestion
*
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -521,6 +614,123 @@ private VersionGCStats gc(long maxRevisionAgeInMillis)
throws IOException {
return stats;
}
+ /**
+ * "Detailed garbage" refers to additional garbage identified as part
of OAK-10199
+ * et al: essentially garbage that in earlier versions of Oak were
ignored. This
+ * includes: deleted properties, revision information within
documents, branch
+ * commit related garbage.
+ * <p/>
Review Comment:
```suggestion
* <p>
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -1853,6 +1853,16 @@ protected <T extends Document> Iterable<T>
queryAsIterable(final Collection<T> c
}
}
+ if (sortBy != null && !sortBy.isEmpty()) {
+ for (String key: sortBy) {
Review Comment:
```suggestion
for (String key : sortBy) {
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -521,6 +614,123 @@ private VersionGCStats gc(long maxRevisionAgeInMillis)
throws IOException {
return stats;
}
+ /**
+ * "Detailed garbage" refers to additional garbage identified as part
of OAK-10199
+ * et al: essentially garbage that in earlier versions of Oak were
ignored. This
+ * includes: deleted properties, revision information within
documents, branch
+ * commit related garbage.
+ * <p/>
+ * TODO: limit this to run only on a singleton instance, eg the
cluster leader
+ * <p/>
Review Comment:
```suggestion
* <p>
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBVersionGCSupport.java:
##########
@@ -85,6 +106,67 @@ public Iterable<NodeDocument> getPossiblyDeletedDocs(final
long fromModified, fi
}
}
+ /**
+ * Returns documents that have a {@link NodeDocument#MODIFIED_IN_SECS}
value
+ * within the given range and are greater than given @{@link
NodeDocument#ID}.
+ * <p>
+ * The two passed modified timestamps are in milliseconds
+ * since the epoch and the implementation will convert them to seconds at
+ * the granularity of the {@link NodeDocument#MODIFIED_IN_SECS} field and
+ * then perform the comparison.
+ * <p/>
Review Comment:
```suggestion
*
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -1853,6 +1853,16 @@ protected <T extends Document> Iterable<T>
queryAsIterable(final Collection<T> c
}
}
+ if (sortBy != null && !sortBy.isEmpty()) {
+ for (String key: sortBy) {
Review Comment:
(whitespace)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java:
##########
@@ -1669,6 +1670,24 @@ SortedMap<Revision, String> getLocalMap(String key) {
return map;
}
+ /**
+ * Returns name of all the properties on this document
+ * <p>
+ * Note: property names returned are escaped
+ * <p/>
Review Comment:
Not needed.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -971,8 +971,8 @@ private <T extends Document> void
toMapBuilder(ImmutableMap.Builder<String, Stri
public static String VERSIONPROP = "__version";
// set of supported indexed properties
- private static final Set<String> INDEXEDPROPERTIES = new
HashSet<String>(Arrays.asList(new String[] { MODIFIED,
- NodeDocument.HAS_BINARY_FLAG, NodeDocument.DELETED_ONCE,
NodeDocument.SD_TYPE, NodeDocument.SD_MAX_REV_TIME_IN_SECS, VERSIONPROP }));
+ private static final Set<String> INDEXEDPROPERTIES = new
HashSet<>(Arrays.asList(MODIFIED,
Review Comment:
I'm not 100% sure why ID wasn't here before. Maybe an oversight, but I woul
not bet on it...
--
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]
