milvus-io / milvus-sdk-java

Java SDK for Milvus.
https://milvus.io
Apache License 2.0
396 stars 165 forks source link

MilvusClientV2Pool.getClient is null sometimes when using client pool #1118

Closed dingle66 closed 2 days ago

dingle66 commented 1 month ago

MilvusClientV2Pool milvusClientV2Pool = new MilvusClientV2Pool(poolConfig, connectConfig); milvusClientV2Pool.getClient is null occurs sometimes!

yhmo commented 1 month ago

https://github.com/milvus-io/milvus-sdk-java/blob/14b9065db1ac5d0d9359a58ddc6e21565cb3a1ec/src/main/java/io/milvus/pool/ClientPool.java#L47

The ClientPool could return null for getClient() method:

    public T getClient(String key) {
        try {
            return clientPool.borrowObject(key);
        } catch (Exception e) {
            System.out.println("Failed to get client, exception: " + e.getMessage());
            return null;
        }
    }

I didn't throw an exception here, do you think it is necessary to throw the error? In my opinion, when user gets a null value, he can check the returned value and recall the getClient() again.

The client.Pool is a GenericKeyedObjectPool. The borrowObject() method internally has a timeout value to wait for a client to be created. The timeout value can be configured by PoolConfig.maxBlockWaitDuration: https://github.com/milvus-io/milvus-sdk-java/blob/14b9065db1ac5d0d9359a58ddc6e21565cb3a1ec/src/main/java/io/milvus/pool/ClientPool.java#L27

The default value of maxBlockWaitDuration is 3 seconds. Sometimes the client could not be created in 3 seconds, which might caused by network issues or milvus server being too busy to respond. You can increase this value to observe.

xiaofan-luan commented 1 month ago

https://github.com/milvus-io/milvus-sdk-java/blob/14b9065db1ac5d0d9359a58ddc6e21565cb3a1ec/src/main/java/io/milvus/pool/ClientPool.java#L47

The ClientPool could return null for getClient() method:

    public T getClient(String key) {
        try {
            return clientPool.borrowObject(key);
        } catch (Exception e) {
            System.out.println("Failed to get client, exception: " + e.getMessage());
            return null;
        }
    }

I didn't throw an exception here, do you think it is necessary to throw the error? In my opinion, when user gets a null value, he can check the returned value and recall the getClient() again.

The client.Pool is a GenericKeyedObjectPool. The borrowObject() method internally has a timeout value to wait for a client to be created. The timeout value can be configured by PoolConfig.maxBlockWaitDuration:

https://github.com/milvus-io/milvus-sdk-java/blob/14b9065db1ac5d0d9359a58ddc6e21565cb3a1ec/src/main/java/io/milvus/pool/ClientPool.java#L27

The default value of maxBlockWaitDuration is 3 seconds. Sometimes the client could not be created in 3 seconds, which might caused by network issues or milvus server being too busy to respond. You can increase this value to observe.

I think the right thing is to throw error. For all java users, exceptions is the only thing need to be handled and return null doesn't really make sense to me

yhmo commented 3 weeks ago

Fixed by this pr: https://github.com/milvus-io/milvus-sdk-java/pull/1170/files The exception thrown from GenericKeyedObjectPool is encapsulated by MilvusClientException and thrown out to users.

yhmo commented 2 days ago

The fix has been released with v2.4.9 and v2.5.0