[Improvement] GravitinoConnectorFactory.java writes to a static field is not thread-safe

[justinmclean]

What would you like to be improved?

GravitinoConnectorFactory.java:65: warning: [StaticGuardedByInstance] Write to static variable should not be guarded by instance lock 'this'
GravitinoPlugin.catalogConnectorManager = catalogConnectorManager;

See https://errorprone.info/bugpattern/StaticGuardedByInstance

How should we improve?

No response

[raghits]

@justinmclean Which branch is this issue on? I'm not seeing this in main branch.

[justinmclean]

It's on the main branch here:
https://github.com/datastrato/gravitino/blob/ca4a8ab1c4907a7e7d35097de66c878a4fb3fdb9/trino-connector/src/main/java/com/datastrato/gravitino/trino/connector/GravitinoConnectorFactory.java#L65

You'll note that is inside a synchronized block and catalogConnectorManager is static.

[raghits]

@justinmclean Assuming I add add a nested sychronized block here such as:
synchronized (GravitinoPlugin.class) { GravitinoPlugin.catalogConnectorManager = catalogConnectorManager; }
This could potentially cause a deadlock if multiple threads are creating connectors. I know this block was added to facilitate testing, could there be a better implementation to intialize GravitinoPlugin? I might need some pointers here. I'm just getting familiar with the project so I might be totally off.

Thanks.