stefan-egli commented on code in PR #1562:
URL: https://github.com/apache/jackrabbit-oak/pull/1562#discussion_r1688100144
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java:
##########
@@ -345,20 +350,31 @@ private Map<String, Object> getVGCSettings() {
return settings;
}
- private void setLongSetting(String propName, long val) {
- setLongSetting(of(propName, val));
+ private void setVGCSetting(final String propName, final Object val) {
+ setVGCSetting(new HashMap<>() {{
Review Comment:
(same as previous)
subclassing HashMap seems heavy, what about using `Map.of()` as was done in
some other cases?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java:
##########
@@ -345,20 +350,31 @@ private Map<String, Object> getVGCSettings() {
return settings;
}
- private void setLongSetting(String propName, long val) {
- setLongSetting(of(propName, val));
+ private void setVGCSetting(final String propName, final Object val) {
+ setVGCSetting(new HashMap<>() {{
+ put(propName, val);
+ }});
}
- private void setStringSetting(String propName, String val) {
- UpdateOp updateOp = new UpdateOp(SETTINGS_COLLECTION_ID, true);
- updateOp.set(propName, val);
+ private void setVGCSetting(final Map<String, Object> propValMap) {
+ final UpdateOp updateOp = new UpdateOp(SETTINGS_COLLECTION_ID, true);
+ setUpdateOp(propValMap, updateOp);
vgc.getDocumentStore().createOrUpdate(Collection.SETTINGS, updateOp);
}
- private void setLongSetting(final Map<String, Long> propValMap) {
- UpdateOp updateOp = new UpdateOp(SETTINGS_COLLECTION_ID, true);
- propValMap.forEach(updateOp::set);
- vgc.getDocumentStore().createOrUpdate(Collection.SETTINGS, updateOp);
+ private void setUpdateOp(final Map<String, Object> propValMap, final
UpdateOp updateOp) {
+ propValMap.forEach((k, v) -> {
+ if (v instanceof Number) updateOp.set(k, ((Number) v).longValue());
+ if (v instanceof String) updateOp.set(k, (String) v);
+ if (v instanceof Boolean) updateOp.set(k, (Boolean) v);
+ });
+ }
+
+ private void updateVGCSetting(final Map<String, Object> propValMap) {
Review Comment:
Noticed a couple issues with this:
* it is not clear in which situation `updateVGCSetting` is used. It seems to
be targeted for `evaluate`. But that method has a complex (if-) structure and
it's not easy to figure out in which cases `updateVGCSetting` is called. Maybe
this is more of a readability comment. Perhaps adding a comment would be a good
improvement thouhg.
* `VersionGCInitTest.lazyInitialize` currently fails since
`updateVGCSetting` _is_ invokved but with a condition that `fullGCTimeStamp`
and `fullGCId` are set. Except it seems those are not initialized. So the
initialization of these must somehow be better covered, I'd suggest first in a
separate test case perhaps, and then in the code.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java:
##########
@@ -266,13 +269,15 @@ public void evaluate(VersionGCStats stats) {
long nextDuration = Math.max(precisionMs, scope.getDurationMs() /
2);
gcmon.info("Limit {} documents exceeded, reducing next collection
interval to {} seconds",
this.maxCollect,
TimeUnit.MILLISECONDS.toSeconds(nextDuration));
-
setLongSetting(VersionGarbageCollector.SETTINGS_COLLECTION_REC_INTERVAL_PROP,
nextDuration);
+
setVGCSetting(VersionGarbageCollector.SETTINGS_COLLECTION_REC_INTERVAL_PROP,
nextDuration);
stats.needRepeat = true;
} else if (!stats.canceled && !stats.ignoredGCDueToCheckPoint &&
!isFullGCDryRun) {
// success, we would not expect to encounter revisions older than
this in the future
- setLongSetting(of(SETTINGS_COLLECTION_OLDEST_TIMESTAMP_PROP,
scope.toMs,
- SETTINGS_COLLECTION_FULL_GC_TIMESTAMP_PROP,
stats.oldestModifiedDocTimeStamp));
- setStringSetting(SETTINGS_COLLECTION_FULL_GC_DOCUMENT_ID_PROP,
stats.oldestModifiedDocId);
+ setVGCSetting(SETTINGS_COLLECTION_OLDEST_TIMESTAMP_PROP,
scope.toMs);
+ updateVGCSetting(new HashMap<>() {{
Review Comment:
subclassing HashMap seems heavy, what about using `Map.of()` as was done in
some other cases?
--
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: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
