[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #550: OAK-9751 : handled cases where path changes in lucene exceeds max bui…
thomasmueller commented on code in PR #550:
URL: https://github.com/apache/jackrabbit-oak/pull/550#discussion_r857640876
##
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/hybrid/LuceneJournalPropertyBuilder.java:
##
@@ -93,6 +93,9 @@ public void addSerializedProperty(@Nullable String json) {
}
if (sizeWithinLimits()) {
indexedNodes.put(path, reader.readString());
+} else {
+// return if max limit reached for builder to avoid
overflow exception
+return;
Review Comment:
I can see why the old code is broken: if the size is exceeded, we don't call
readString(), but continue and try to read a comma...
But I also don't like the new code: we wouldn't detect any errors in the
Json format afterwards... Which I would expect we do.
Instead of return, what about:
```
String x = reader.readString()
if (sizeWithinLimits()) {
indexedNodes.put(path, x);
}
```
Sure, one could argue it is more efficient to return, but I think the side
effect of _not_ parsing the rest in this case is worse than the performance hit
(which is unlikely to be measurable).
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #550: OAK-9751 : handled cases where path changes in lucene exceeds max bui…
thomasmueller commented on code in PR #550:
URL: https://github.com/apache/jackrabbit-oak/pull/550#discussion_r857640876
##
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/hybrid/LuceneJournalPropertyBuilder.java:
##
@@ -93,6 +93,9 @@ public void addSerializedProperty(@Nullable String json) {
}
if (sizeWithinLimits()) {
indexedNodes.put(path, reader.readString());
+} else {
+// return if max limit reached for builder to avoid
overflow exception
+return;
Review Comment:
I can see why the old code is bad: if the size is exceeded, we don't call
readString(), but continue and try to read a comma...
But I also don't like the new code: we wouldn't detect any errors in the
Json format afterwards... Which I would expect we do.
Instead of return, what about:
```
String x = reader.readString()
if (sizeWithinLimits()) {
indexedNodes.put(path, x);
}
```
Sure, one could argue it is more efficient to return, but I think the side
effect of _not_ parsing the rest in this case is worse than the performance hit
(which is unlikely to be measurable).
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
