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

On StorageClient close, should also release meta client. #344

Merged
merged 3 commits into from
Sep 17, 2021
Merged

On StorageClient close, should also release meta client. #344

merged 3 commits into from
Sep 17, 2021

Conversation

heziai
Copy link
Contributor

@heziai heziai commented Sep 6, 2021

If User just use StorageClient, don't use MetaManager, When close StorageClient, then meta client is leaked, and in general, user don't know to use MetaManager.getMetaManager(address).close() to release meta client.
In my opinion, because MetaManager is a single instance, it should be just used at StorageClient, should be moved to storage package. If not, it shouldn't be single instance.

@Nicole00
Copy link
Contributor

Nicole00 commented Sep 6, 2021

Thanks for your contribution.

  1. MetaManager is not just used by StorageClient, it's a manager for schema cache. When users use it, they only need to instantiate MetaManager once.
  2. When StorageClient is closed, the MetaManager should also be closed too. That's a great catch. So we need to fix it through adding metaManager.close in StorageClient#close().

@heziai
Copy link
Contributor Author

heziai commented Sep 6, 2021

If users use StorageClient and MetaManager at the same time, close StorageClient maybe has problem, as users don't want to close MetaManager.
I think, MetaCache Single Intance could be build by user.

@Nicole00
Copy link
Contributor

Nicole00 commented Sep 7, 2021

If users use StorageClient and MetaManager at the same time, close StorageClient maybe has problem, as users don't want to close MetaManager.
I think, MetaCache Single Intance could be build by user.

If users use both StorageClient and MetaManager, then close it after both finished.

@heziai
Copy link
Contributor Author

heziai commented Sep 7, 2021

Actually, in my projects, there are multi nebula clusters, in one java process, will connect multi clusters, include gragh、storage、meta, but now, the MetaManager is not supported, because it's single instance and just support a cluster addresses.
On the other hand, the single instance is not suitable to reinit and close frequently. But my application is dynamic to reinit and close gragh, storage, meta clients.

@heziai
Copy link
Contributor Author

heziai commented Sep 7, 2021

If users use StorageClient and MetaManager at the same time, close StorageClient maybe has problem, as users don't want to close MetaManager.
I think, MetaCache Single Intance could be build by user.

If users use both StorageClient and MetaManager, then close it after both finished.

Users don't know all about this easily. The core problem is, StorageClient has used MetaManager(should be released), but users don't know StorageClient has used MetaManager . If MetaManager is not single instance, then whatever, you deal with release opration, not affect users.

@Nicole00
Copy link
Contributor

Nicole00 commented Sep 8, 2021

Actually, in my projects, there are multi nebula clusters, in one java process, will connect multi clusters, include gragh、storage、meta, but now, the MetaManager is not supported, because it's single instance and just support a cluster addresses.
On the other hand, the single instance is not suitable to reinit and close frequently. But my application is dynamic to reinit and close gragh, storage, meta clients.

Can you use MetaClient to get schema information in your application? Your point is right, Maybe we should guide users to use MetaClient, and as MetaManager an internal tool.

@heziai
Copy link
Contributor Author

heziai commented Sep 8, 2021

Actually, in my projects, there are multi nebula clusters, in one java process, will connect multi clusters, include gragh、storage、meta, but now, the MetaManager is not supported, because it's single instance and just support a cluster addresses.
On the other hand, the single instance is not suitable to reinit and close frequently. But my application is dynamic to reinit and close gragh, storage, meta clients.

Can you use MetaClient to get schema information in your application? Your point is right, Maybe we should guide users to use MetaClient, and as MetaManager an internal tool.

For now, haven't use MetaClient, but use StorageClient, and need connect multi clusters.

@heziai
Copy link
Contributor Author

heziai commented Sep 17, 2021

Will this PR be merged? If not, how to intend to satisfy the needs of dynamic init/close and multi clusters support?

@Nicole00
Copy link
Contributor

Will this PR be merged? If not, how to intend to satisfy the needs of dynamic init/close and multi clusters support?

Yes, your solution is reasonable, we will merge it after rerunning the build process. Thanks for your contribution again.

@Nicole00 Nicole00 merged commit 4b9c617 into vesoft-inc:master Sep 17, 2021
@heziai heziai mentioned this pull request Oct 13, 2021
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.

2 participants