Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAK-9757 : increased node name limit for mongo 4.2 version #560

Merged
merged 22 commits into from Jun 13, 2022

Conversation

rishabhdaim
Copy link
Contributor

No description provided.

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. I would say that if we need to modify 109 files, there's something very wrong with this change.

@reschke
Copy link
Contributor

reschke commented May 5, 2022

Idea: Move (or copy) the util functions to the DocumentStore API, where they have access to metadata.

@rishabhdaim
Copy link
Contributor Author

rishabhdaim commented May 6, 2022

Thanks @reschke, that's a good idea to move util function to DocumentStore and that would help us avoiding in calling metadata multiple times.
Let me make changes and raise the PR again, but I doubt that would restrict the code changes to less file since most of the changes are in tests cases and the changes are to pass the parameters to utils class.

@rishabhdaim rishabhdaim force-pushed the OAK-9757 branch 2 times, most recently from d2409ef to decf195 Compare May 6, 2022 11:13
@mreutegg
Copy link
Contributor

mreutegg commented May 6, 2022

I suggest we take a slightly different approach. To me it feels wrong that Utils.isLongPath() is currently responsible for detecting when a node name is too long. I think this is the reason why so many other code changes are necessary. E.g. the method is then used in Utils.getIdFromPath(), which in turn is used in many places. But in many cases the check for the node name length is not actually needed. I my view it would be better if the node name length check is moved out of Utils.isLongPath() to a place were it is really needed. That is, when a new node is added through the DocumentNodeStore or created with an UpdateOp on the DocumentStore.

@mreutegg
Copy link
Contributor

mreutegg commented May 6, 2022

Also note that checks fails because two new files for classes MongoVersion and MongoVersionTest do not have a license header.

@rishabhdaim
Copy link
Contributor Author

@mreutegg

To me it feels wrong that Utils.isLongPath() is currently responsible for detecting when a node name is too long

Yes, I think it should be other way around i.e. node name too long should check whether path is long or not.
I think, we should move out the logic to check node name too long to a separate method and call Utils.isLongPath() from it since node name too long make sense only for long paths.
/*** The maximum size a node name, in bytes. This is only a problem for long path.*/ int NODE_NAME_LIMIT = Integer.getInteger("oak.nodeNameLimit", 150);
As mentioned by you, we should move whole logic for node name too long here https://github.com/apache/jackrabbit-oak/blob/trunk/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeState.java#L417 and then we should be able to reduce the number of changed files significantly.

@rishabhdaim rishabhdaim force-pushed the OAK-9757 branch 8 times, most recently from 9c61994 to 5ed7a64 Compare May 11, 2022 08:17
Rishabh Kumar and others added 3 commits May 24, 2022 12:44
Copy link
Contributor

@mreutegg mreutegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look pretty good now but then I noticed a problem.

The new default method on the DocumentStore interfaces is not overridden in wrapper classes like LeaseCheckDocumentStoreWrapper. This means the increased node name capability of a MongoDocumentStore may be hidden behind one of those wrapper classes.

Copy link
Contributor

@stefan-egli stefan-egli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, lgtm now.
(I agree, the other DocumentStore classes in /src/test do not necessarily have to implement getNodeNameLimit - so that's fine to leave those out)

@mreutegg mreutegg merged commit b2f8598 into apache:trunk Jun 13, 2022
@rishabhdaim rishabhdaim deleted the OAK-9757 branch June 13, 2022 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants