OAK-12352: Include diff indexes in the index definition status printer - #3078
OAK-12352: Include diff indexes in the index definition status printer#3078chibulcuteanu wants to merge 1 commit into
Conversation
/oak:index/diff.index and /oak:index/diff.index.optimizer are not oak:QueryIndexDefinition nodes, so IndexPathService does not return them and they were missing from the oak-index-defn status printer output. Append them explicitly, inlining their diff.json payload as JSON instead of a base64 blob. All other properties and child nodes render as before.
| NodeState idxState = NodeStateUtils.getNode(root, indexPath); | ||
| createSerializer(json).serialize(idxState); | ||
| } | ||
| // The "diff" indexes (diff.index / diff.index.optimizer) are not oak:QueryIndexDefinition nodes, so they |
There was a problem hiding this comment.
Detail (what confused Claude): In oak-doc/src/site/markdown/query/indexing.md we write:
The diff index consists of:
/oak:index/diff.index
- type: disabled
- jcr:primaryType: oak:QueryIndexDefinition
+ diff.json (nt:file)
This contradicts the jcr:primaryType actually used and recommended in the customer facing doc, but if following the oak doc the primary type is oak:QueryIndexDefinition and therefore picked up in the index printer.
There was a problem hiding this comment.
But if one uses jcr:primaryType: oak:QueryIndexDefinition, we now end up with a duplicate key /oak:index/diff.index, so maybe worth changing the doc.
There was a problem hiding this comment.
agree. @thomasmueller , I will also create a small PR to change the docs.
| serializer.serialize(p); | ||
| } | ||
| // non-hidden child nodes other than diff.json, rendered normally | ||
| for (ChildNodeEntry child : idxState.getChildNodeEntries()) { |
There was a problem hiding this comment.
Is this running with the right order of child nodes? Or does it even matter here?
(I struggle a bit to understand where OAK requires correct node ordering and where not)
| continue; | ||
| } | ||
| json.key(childName); | ||
| createSerializer(json).serialize(child.getNodeState()); |
There was a problem hiding this comment.
Is the serializer so stateful that we need to recreate it ?
| // The "diff" indexes (diff.index / diff.index.optimizer) are not oak:QueryIndexDefinition nodes, so they | ||
| // are not returned by the IndexPathService and would otherwise be missing from the output. Add them | ||
| // explicitly, rendering their diff.json payload as inline JSON so the pending diff is readable. | ||
| for (String name : new String[] {DiffIndexMerger.DIFF_INDEX, DiffIndexMerger.DIFF_INDEX_OPTIMIZER}) { |
There was a problem hiding this comment.
IMHO, there is a lot of logic for something that should be a printer
There was a problem hiding this comment.
When I see this kind of "work around", it very often means there is a design flaw somewhere. It's likely not the issue of this PR, but something else (the complexity of this PR is a symptom).
Before /oak:index/ contained a single type of object oak:QueryIndexDefinition. Now it can contain 2 types of objects oak:QueryIndexDefinition and these diff.index or diff.index.optimizer for those 2 it's not the type that is important but their name.
Before, printing was a natural walk through the tree outputting what ever was on it's path.
Maybe we can make this "printing" a bit cleaner by splitting this method up:
loop on the indexPathService.getIndexPaths() -> call a auxilary "printIndexDefinitionNode" method for this path
Move the logic to determine which diffIndexPaths to the indexPathService (where we add a method .getDiffPaths())
loop on those results and internally call an auxilary "printDiffNode" method (which is basically serializeDiffIndex, see my comments there).
| // The "diff" indexes (diff.index / diff.index.optimizer) are not oak:QueryIndexDefinition nodes, so they | ||
| // are not returned by the IndexPathService and would otherwise be missing from the output. Add them | ||
| // explicitly, rendering their diff.json payload as inline JSON so the pending diff is readable. | ||
| for (String name : new String[] {DiffIndexMerger.DIFF_INDEX, DiffIndexMerger.DIFF_INDEX_OPTIMIZER}) { |
There was a problem hiding this comment.
When I see this kind of "work around", it very often means there is a design flaw somewhere. It's likely not the issue of this PR, but something else (the complexity of this PR is a symptom).
Before /oak:index/ contained a single type of object oak:QueryIndexDefinition. Now it can contain 2 types of objects oak:QueryIndexDefinition and these diff.index or diff.index.optimizer for those 2 it's not the type that is important but their name.
Before, printing was a natural walk through the tree outputting what ever was on it's path.
Maybe we can make this "printing" a bit cleaner by splitting this method up:
loop on the indexPathService.getIndexPaths() -> call a auxilary "printIndexDefinitionNode" method for this path
Move the logic to determine which diffIndexPaths to the indexPathService (where we add a method .getDiffPaths())
loop on those results and internally call an auxilary "printDiffNode" method (which is basically serializeDiffIndex, see my comments there).
| * rendered as base64 blobs, for backward compatibility. | ||
| */ | ||
| private void serializeDiffIndex(JsopBuilder json, NodeState idxState) { | ||
| json.object(); |
There was a problem hiding this comment.
I don't like so much that we have walking logic in this class and them more walking logic in the serializer class.
The role of this class is to delegate to JsonSerializer (which consists in walking a NodeState and outputting in JSON what it finds). So it does the walking... we shouldn't be doing it here. Why are we doing it here ? Because we want a special processing of a certain parts : the inlining of the diff.json file object. Why don't we just update the serializer (eventually with a config) to add inlining of certain nodes and the configure the serializer with that option here ?
Here we would not need the 2 extra loops as this would be taken care of by the serializer.
3 participants
/oak:index/diff.index and /oak:index/diff.index.optimizer are not oak:QueryIndexDefinition nodes, so IndexPathService does not return them and they were missing from the oak-index-defn status printer output. Append them explicitly, inlining their diff.json payload as JSON instead of a base64 blob. All other properties and child nodes render as before.