gravitino icon indicating copy to clipboard operation
gravitino copied to clipboard

[Improvement] Improve GravitinoCatalogManager close method

Open justinmclean opened this issue 1 year ago • 7 comments

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 avatar Apr 12 '24 06:04 justinmclean

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

xiaozcy avatar Apr 22 '24 08:04 xiaozcy

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

FANNG1 avatar Apr 29 '24 00:04 FANNG1

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.

justinmclean avatar Apr 29 '24 00:04 justinmclean

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

FANNG1 avatar Apr 29 '24 01:04 FANNG1

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.

yuqi1129 avatar Apr 29 '24 02:04 yuqi1129

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

justinmclean avatar Apr 29 '24 02:04 justinmclean

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.

shaofengshi avatar Aug 08 '24 03:08 shaofengshi