Skip to content

[IOTDB-1148]fix the client leak of client pool&use remote schema cache when check timeseries exist or not#2635

Merged
mychaow merged 8 commits intoapache:masterfrom
neuyilan:apache_master_0204_fix_client_pool_leak
Feb 20, 2021
Merged

[IOTDB-1148]fix the client leak of client pool&use remote schema cache when check timeseries exist or not#2635
mychaow merged 8 commits intoapache:masterfrom
neuyilan:apache_master_0204_fix_client_pool_leak

Conversation

@neuyilan
Copy link
Member

@neuyilan neuyilan commented Feb 5, 2021

@neuyilan neuyilan marked this pull request as ready for review February 5, 2021 06:17
@neuyilan neuyilan requested a review from jt2594838 February 5, 2021 06:17
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 5, 2021

@mychaow mychaow added the bug Something isn't working label Feb 5, 2021
@wangchao316
Copy link
Member

Generally, the connection pool is used to obtain connections from the connection pool. After the close connection is used, the close connection only puts the connection back to the connection pool. The connection pool maintains the lifecycle of the connection.

@neuyilan
Copy link
Member Author

neuyilan commented Feb 7, 2021

Generally, the connection pool is used to obtain connections from the connection pool. After the close connection is used, the close connection only puts the connection back to the connection pool. The connection pool maintains the lifecycle of the connection.

Actually, it's not exactly right. Not all client life cycles are processed in the client pool. When getting a client for remote process use, there may be problems. For example, a client may fail to connect due to network delay or server downtime. At this time, we need to close the connection. When you put the client in the client pool, if you find that the connection is closed, you will not put it in the pool. The role of the client pool is to save some clients that have created connections. When you want to connect to a server, you don't need to create connections again, which saves the overhead of creating and closing connections frequently.

Comment on lines +418 to +424
public synchronized boolean containsKey(PartialPath key) {
return cache.containsKey(key);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to mix synchronized and lock?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all methods in RemoteMetaCache are synchronized, it is safe to modify the cache. However, in CMManager, functions in RemoteMetaCache may be called many times in a function, which may be due to the serialization of cache operations in CMManager, so the lock is added?

Therefore, I think in CMManager, whether the cache operation is locked or not is OK. Locking can ensure that all cache operations in CMManager are serial. However, since RemoteMetaCache itself is thread-safe, maybe it's also OK not to lock it.

@HTHou HTHou added the Module - Cluster PRs for the cluster module label Feb 18, 2021
@neuyilan neuyilan force-pushed the apache_master_0204_fix_client_pool_leak branch from 61e07c3 to c79f8ef Compare February 19, 2021 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Module - Cluster PRs for the cluster module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants