OAK-11025 Silence more warnings for ordered properties

[thomasmueller]

No description provided.

[nfsantos]

I think we should treat each of the messages separately. As it is, this implementation may totally suppress some classes of warnings. A message like "Not a date string" might never be shown if it just happens to always occur when the logging is being suppressed. Maybe we can use this class, which already does something like what we want:

https://github.com/apache/jackrabbit-oak/blob/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/LogSilencer.java

[thomasmueller]

I think we should treat each of the messages separately.

At first, I also thought about doing something similar: having a separate delay for each property. But that would complicate the code, and in my view unnecessarily: We don't have any evidence that someone (except us) is looking at these warnings. If someone is looking at the warnings, then he would take action, no matter what message.

I had a look at the LogSilencer, but it seems it is not protected against multi-threaded access to the cache. I don't want to risk running out of memory.

So I think the patch should be kept as-is.

[nfsantos]

I don't agree that we should suppress altogether some types of errors. Either the error message is important and should always be logged (although removing duplicates), or else it is not important and it should not be logged at all. But a probabilistic approach where the message is logged in some runs but not in other runs, depending only on chance does not seem to be correct.

From what I saw, the LogSilencer is thread-safe and supports bounding the memory usage, so there is no risk of OOME.

jackrabbit-oak/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/LogSilencer.java

Lines 34 to 45 in 396b3c4

	private static final int DEFAULT_CACHE_SIZE = 64;
	private final int cacheSize;
	private final long silenceMillis;
	@SuppressWarnings("serial")
	private final Map<String, Long> silences = Collections.synchronizedMap(
	new LinkedHashMap<String, Long>() {
	protected final boolean removeEldestEntry(Map.Entry<String, Long> eldest) {
	return size() > cacheSize;
	}
	});

The only thing I don't like so much about the LogSilencer is that in this case it would be more useful to count the number of times a message is suppressed and then print a warning like:

(this message was repeated n times).

So that we would know how widespread is the particular issue (if a few dozen times in a run, then ignore, if it appears millions of times, may be worth to investigate).

[thomasmueller]

I don't agree that we should suppress altogether some types of errors

Well, the patch doesn't. It just avoid filling the log file.

Either the error message is important and should always be logged (although removing duplicates), or else it is not important and it should not be logged at all.

I don't see this as a binary thing. We do log a warning if we find a problem, but we need to avoid logging too much. That is what the patch does.

From what I saw, the LogSilencer is thread-safe

Ah I didn't see Collections.synchronizedMap sorry.

OK I'll try to use it.

[nfsantos]

This could be simplified a bit, something like:

List<String> errorMessagePrefixes = List.of("Not a date string", "Unable to parse the provided date field", "For input string");
...
String key = errorMessagePrefixes.stream().find(t -> message.startsWith(t))...