nfsantos commented on code in PR #871:
URL: https://github.com/apache/jackrabbit-oak/pull/871#discussion_r1136668528
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java:
##########
@@ -330,9 +334,14 @@ private void collectIndexEditors(NodeBuilder definitions,
// then we don't need to handle missing handler
if (definition.hasProperty(ASYNC_PROPERTY_NAME) &&
rootState.async == null) {
if (!TYPE_DISABLED.equals(type)) {
- log.warn("Missing provider for nrt/sync index: {}
(rootState.async: {}). " +
- "Please note, it means that index data
should be trusted only after this index " +
- "is processed in an async indexing
cycle.", definition, rootState.async);
+ long now = System.currentTimeMillis();
+ if (now > lastMissingProviderMessageTime + 60 *
1000) {
+ lastMissingProviderMessageTime = now;
Review Comment:
The read-test-update of `lastMissingProviderMessageTime` is not atomic, so
two or more threads may pass the check and print the log more or less at the
same time. If this is an intentional loosening of the specification of "log at
most every minute" for the sake of performance, please write it in the
comments. Otherwise, I think it is more clear and clean if the implementation
did not have race conditions that allow it to break the intention that is
apparent from the code (log at most once a minute).
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java:
##########
@@ -330,9 +334,14 @@ private void collectIndexEditors(NodeBuilder definitions,
// then we don't need to handle missing handler
if (definition.hasProperty(ASYNC_PROPERTY_NAME) &&
rootState.async == null) {
if (!TYPE_DISABLED.equals(type)) {
- log.warn("Missing provider for nrt/sync index: {}
(rootState.async: {}). " +
- "Please note, it means that index data
should be trusted only after this index " +
- "is processed in an async indexing
cycle.", definition, rootState.async);
+ long now = System.currentTimeMillis();
Review Comment:
`System.currentTimeMillis()` is not guaranteed to measure correctly the
passage of time.
https://www.geeksforgeeks.org/java-system-nanotime-vs-system-currenttimemillis/
> It may give wrong results if the user changes the system time, hits a leap
second or there are changes in NTP sync.
Even if in this use case it is not important if from time to time log
messages are logged more than once a second or less frequently than once a
second, I think we should be strictly correct, not to create confusion.
--
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]
