[#2167] refactor: refactor TestGravitinoConnector to enable StaticGuardedByInstance error-prone #2514
Conversation
jerryshao
requested review from
diqiu50 and
yuqi1129
and removed request for
diqiu50
| server.setCatalogConnectorManager(GravitinoPlugin.catalogConnectorManager); | ||
|
|
||
| CatalogConnectorManager catalogConnectorManager = | ||
| gravitinoPlugin.getCatalogConnectorManager(); |
There was a problem hiding this comment.
I think there is a real Gravitino client in the CatalogConnectorManager, which may be used to load catalogs. You can pass the mocked Gravitino client to the TestGravitinoFactory to initialize the CatalogConnectorManager.
There was a problem hiding this comment.
@diqiu50 Sorry, I don't get it. I have already passed the mocked gravitino client to CatalogConnectorManager:
catalogConnectorManager.setGravitinoClient(gravitinoClient);
Can you please provide a more detailed explanation? Thanks.
There was a problem hiding this comment.
The code snippet:
76 queryRunner.createCatalog("test1", "gravitino", properties);
calls the GravitinoConnector.create() function, which instantiates a CatalogConnectorManager. When start() is invoked on the CatalogConnectorManager, it creates an instance of GravitinoClient. This instance is a real GravitinoClient object.
Later, the line:
81 catalogConnectorManager.setGravitinoClient(gravitinoClient);
replaces the instance with a mocked GravitinoClient.
At this point, the real GravitinoClient may still be performing background tasks.
| } | ||
|
|
||
| public CatalogConnectorManager getCatalogConnectorManager() { | ||
| return catalogConnectorManager; |
There was a problem hiding this comment.
Why don't you return factory.getCatalogConnectorManager() directly as it also depends on method GravitinoConnectorFactory#create to initialize catalogConnectorManager;
There was a problem hiding this comment.
.Indeed. I will fix it.
|
It's okay with me for the current changes. @diqiu50, do you have any further comments? |
|
@coolderli |
…ticGuardedByInstance error-prone
…ticGuardedByInstance error-prone (apache#2514) <!-- 1. Title: [#<issue>] <type>(<scope>): <subject> Examples: - "[apache#123] feat(operator): support xxx" - "[apache#233] fix: check null before access result in xxx" - "[MINOR] refactor: fix typo in variable name" - "[MINOR] docs: fix typo in README" - "[apache#255] test: fix flaky test NameOfTheTest" Reference: https://www.conventionalcommits.org/en/v1.0.0/ 2. If the PR is unfinished, please mark this PR as draft. --> ### What changes were proposed in this pull request? `StaticGuardedByInstance` amis to avoid `public static` fields. Because this field is not thread-safe. See more details at https://errorprone.info/bugpattern/StaticGuardedByInstance. In this MR, I remove some test code in `com.datastrato.gravitino.trino.connector.GravitinoPlugin` and `com.datastrato.gravitino.trino.connector.GravitinoConnectorFactory`. At the same time, I enable `StaticGuardedByInstance` error-prone. ### Why are the changes needed? - Fix: apache#2167 ### Does this PR introduce _any_ user-facing change? - no ### How was this patch tested? - original unit tests.
What changes were proposed in this pull request?
StaticGuardedByInstanceamis to avoidpublic staticfields. Because this field is not thread-safe. See more details at https://errorprone.info/bugpattern/StaticGuardedByInstance.In this MR, I remove some test code in
com.datastrato.gravitino.trino.connector.GravitinoPluginandcom.datastrato.gravitino.trino.connector.GravitinoConnectorFactory. At the same time, I enableStaticGuardedByInstanceerror-prone.Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?