Skip to content

GH-1246. Asynchronously initialize cache before reading II #1250

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

Merged

Conversation

kotman12
Copy link
Contributor

@kotman12 kotman12 commented Mar 5, 2025

This is an alternative, simpler implementation to #1247 with the same motivation, namely to solve #1246

@tisonkun
Copy link
Member

Thanks for preparing this patch! I'd review it by the end of next week (sorry this week is a busy week for me >_<)

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Looks good and very tidy.

Thanks a lot for your contribution!

@tisonkun
Copy link
Member

I'm waiting for other committers' review (cc @eolivelli @kezhuw @Randgalt @cammckenzie) but would otherwise merge this patch in the next week if there is no more objection.

@kotman12
Copy link
Contributor Author

Thanks for taking a look @tisonkun!

…ed/TestCachedModeledFramework.java

Co-authored-by: Kezhu Wang <[email protected]>
@kotman12
Copy link
Contributor Author

Hey .. I just did a walk-through of the code again and have a question. Is applying the filter:

e -> !e.getKey().isRoot()
                                && e.getKey().parent().equals(client.modelSpec().path())

safe to do from ::list()? I realize I added that behavior by deferring to the newly created internalChildren from within list which applies this filter that wasn't applied before. I am worried this is a regression not caught by the tests.

If it is ok then it is probably ok to remove the now unused methods in `ModeledCachedImpl:

    ZPath basePath() {
        return basePath;
    }

    Map<ZPath, ZNode<T>> currentChildren() {
        return currentChildren(basePath);
    }

Either way I think a change is due ... let me know what you think!

@tisonkun
Copy link
Member

tisonkun commented Mar 21, 2025

@kotman12 Let's removed the unused method in a follow-up PR.

I'm going to merge this PR after CI green.

@tisonkun tisonkun merged commit 190cd65 into apache:master Mar 21, 2025
10 checks passed
@tisonkun
Copy link
Member

Merged. Thanks for your contribution @kotman12!

Would you provide a follow-up to remove the unused methods mentioned above?

@kotman12
Copy link
Contributor Author

@tisonkun I have done so here #1257 .. looking at the past behavior of ::list as well as re-reading the API description I think I did inadvertently break the contract with this PR. Hopefully the change to fix it makes sense.

kezhuw pushed a commit that referenced this pull request Mar 26, 2025
…k::list (#1257)

#1250 introduced a bug where CachedModeledFramework::list is no longer returning all descendants of the basePath of the underlying cache and is instead returning the direct children of the current path of the client.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants