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

[Improvement] Improve GravitinoCatalogManager close method #2911

Open
justinmclean opened this issue Apr 12, 2024 · 7 comments · May be fixed by #3096
Open

[Improvement] Improve GravitinoCatalogManager close method #2911

justinmclean opened this issue Apr 12, 2024 · 7 comments · May be fixed by #3096
Labels
good first issue Good for newcomers improvement Improvements on everything

Comments

@justinmclean
Copy link
Member

What would you like to be improved?

GravitinoCatalogManager's close method mixes static and volatile variables in a nonstatic method and that may cause issues. Correctly updating a static field from a non-static method can be tricky to get right and could easily lead to bugs if there are multiple threads.

How should we improve?

See if this can be improved to be more thread safe.

@justinmclean justinmclean added good first issue Good for newcomers improvement Improvements on everything labels Apr 12, 2024
@xiaozcy
Copy link
Contributor

xiaozcy commented Apr 22, 2024

I created a PR(#3096) for this, could you take a look at it?

@jerryshao jerryshao added this to the Gravitino June Release milestone Apr 24, 2024
@FANNG1
Copy link
Contributor

FANNG1 commented Apr 29, 2024

it's not supposed to work in the multi-thread environment, so I prefer not to change it.

@justinmclean
Copy link
Member Author

Is the fact that it won't work in a multi-thread environment documented anywhere? That seems like an omission that a user would want to know about.

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 29, 2024

There is no document for this, maybe we could add some comment to the class file, WDYT?

@yuqi1129
Copy link
Contributor

See if this can be improved to be more thread safe.

Is there a need to support multi-threading? It currently does not support multi-threading.

@justinmclean
Copy link
Member Author

Perhaps I'm misisng something, but I assume users expect this to support muti-threading.

@shaofengshi
Copy link
Contributor

I agree with Justin, we couldn't stop user/client (especially it is a flink connector) from using it in multi-threading, so the code should be robust, especially that is too complex to achieve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers improvement Improvements on everything
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants