stefan-egli commented on code in PR #956:
URL: https://github.com/apache/jackrabbit-oak/pull/956#discussion_r1218171851
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java:
##########
@@ -101,6 +108,22 @@ public VersionGCRecommendations(long maxRevisionAgeMs,
Checkpoints checkpoints,
TimeInterval scope = new TimeInterval(oldestPossible, Long.MAX_VALUE);
scope = scope.notLaterThan(keep.fromMs);
+ detailedGCTimestamp =
settings.get(SETTINGS_COLLECTION_DETAILED_GC_TIMESTAMP_PROP);
+ if (detailedGCTimestamp == 0) {
+ if (log.isDebugEnabled()) {
+ log.debug("No detailedGCTimestamp found, querying for the
oldest deletedOnce candidate");
+ }
+ oldestPossibleFullGC = vgc.getOldestModifiedTimestamp(clock) - 1;
+ if (log.isDebugEnabled()) {
+ log.debug("detailedGCTimestamp found: {}",
Utils.timestampToString(oldestPossibleFullGC));
+ }
Review Comment:
I see that in the existing code above we're also only doing a `log.debug` -
but I would actually argue that in both cases it could very well at least be a
`log.info` for visibility. wdyt?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBVersionGCSupport.java:
##########
@@ -239,6 +268,33 @@ public long getOldestDeletedOnceTimestamp(Clock clock,
long precisionMs) {
}
}
+ /**
+ * Retrieve the time of the oldest modified document.
+ *
+ * @param clock System Clock
+ * @return the timestamp of the oldest modified document.
+ */
+ @Override
+ public long getOldestModifiedTimestamp(Clock clock) {
Review Comment:
(same comment about unit test coverage)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStoreJDBC.java:
##########
@@ -715,6 +715,7 @@ private PreparedStatement prepareQuery(Connection
connection, RDBTableMetaData t
}
if (sortBy != null) {
+ // FIXME : order should be determined via sortBy field
Review Comment:
should this be rather a Jira ticket? (to what extend is it related to
OAK-10199, would something fail based on this on RDB?)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java:
##########
@@ -101,6 +108,22 @@ public VersionGCRecommendations(long maxRevisionAgeMs,
Checkpoints checkpoints,
TimeInterval scope = new TimeInterval(oldestPossible, Long.MAX_VALUE);
scope = scope.notLaterThan(keep.fromMs);
+ detailedGCTimestamp =
settings.get(SETTINGS_COLLECTION_DETAILED_GC_TIMESTAMP_PROP);
+ if (detailedGCTimestamp == 0) {
+ if (log.isDebugEnabled()) {
Review Comment:
`log.isDebugEnabled()` doesn't seem necessary here (but with `log.info`
change below it would entirely become obsolete)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCSupport.java:
##########
@@ -160,6 +175,30 @@ public long getOldestDeletedOnceTimestamp(Clock clock,
long precisionMs) {
return ts;
}
+ /**
+ * Retrieve the time of the oldest modified document.
+ *
+ * @return the timestamp of the oldest modified document.
+ */
+ public long getOldestModifiedTimestamp(final Clock clock) {
Review Comment:
this method should have a dedicated (few) unit test(s)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoVersionGCSupport.java:
##########
@@ -193,6 +225,38 @@ public void apply(BasicDBObject document) {
return result.get(0);
}
+ /**
+ * Retrieve the time of the oldest modified document.
+ *
+ * @param clock System Clock to measure time in accuracy of millis
+ * @return the timestamp of the oldest modified document.
+ */
+ @Override
+ public long getOldestModifiedTimestamp(final Clock clock) {
Review Comment:
this method should ideally have a (few) unit test(s)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBVersionGCSupport.java:
##########
@@ -85,6 +91,29 @@ public Iterable<NodeDocument> getPossiblyDeletedDocs(final
long fromModified, fi
}
}
+ /**
+ * Returns documents that have a {@link NodeDocument#MODIFIED_IN_SECS}
value
+ * within the given range .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.
+ *
+ * @param fromModified the lower bound modified timestamp (inclusive)
+ * @param toModified the upper bound modified timestamp (exclusive)
+ * @param limit the limit of documents to return
+ * @return matching documents.
+ */
+ @Override
+ public Iterable<NodeDocument> getModifiedDocs(final long fromModified,
final long toModified, final int limit) {
Review Comment:
would also consider a unit test here
--
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]
