nfsantos commented on code in PR #1108:
URL: https://github.com/apache/jackrabbit-oak/pull/1108#discussion_r1319920144
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java:
##########
@@ -362,10 +376,15 @@ private boolean addTypedOrderedFields(D doc,
PropertyDefinition pd) {
// Ignore and warn if property multi-valued as not supported
if (property.getType().isArray()) {
- log.warn(
- "[{}] Ignoring ordered property {} of type {} for path {}
as multivalued ordered property not supported",
- getIndexName(), pname,
- Type.fromTag(property.getType().tag(), true), path);
+ // Log the warning for every 1000 (default to 1000 but
configurable) occurrences
+ // We could miss certain paths being logged here since the
DocumentMaker is created for each node state for each index.
+ // But ideally a warning with the property in question should
suffice.
+ if (!throttleWarnLogs ||
WARN_LOG_COUNTER_MV_ORDERED_PROPERTY.incrementAndGet() == 1 ||
WARN_LOG_COUNTER_MV_ORDERED_PROPERTY.get() % throttleWarnLogThreshold == 0) {
+ log.warn(
+ "[{}] Ignoring ordered property {} of type {} for path
{} as multivalued ordered property not supported",
+ getIndexName(), pname,
+ Type.fromTag(property.getType().tag(), true), path);
+ }
Review Comment:
Won't this also risk missing some properties? Imagine there are several
properties that fall in this warning. We could end up logging only for a few of
the properties and not for others. From what I understood, the important
information here is what properties are misconfigured.
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java:
##########
@@ -79,6 +90,9 @@ public FulltextDocumentMaker(@Nullable
FulltextBinaryTextExtractor textExtractor
this.path = checkNotNull(path);
this.logWarnStringSizeThreshold =
Integer.getInteger(WARN_LOG_STRING_SIZE_THRESHOLD_KEY,
DEFAULT_WARN_LOG_STRING_SIZE_THRESHOLD_VALUE);
+ this.throttleWarnLogs = Boolean.getBoolean(THROTTLE_WARN_LOGS_KEY);
+ this.throttleWarnLogThreshold =
Integer.getInteger(THROTTLE_WARN_LOGS_THRESHOLD_KEY,
+ DEFAULT_THROTTLE_WARN_LOGS_THRESHOLD_VALUE);
Review Comment:
If I understood correctly, a new instance of this class is created for every
node that is indexed. Adding two more instance fields here will increase memory
and CPU usage during indexing, because this constructor is on the critical path
of indexing. Aren't these values constant? Can we make them static variables?
--
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]
