MilvusClientV2Pool getClient method and PoolClientFactory create method should throw exception

[yin-bp]

MilvusClientV2Pool getClient(String key) should throw the exception when exception occured:

public MilvusClientV2 getClient(String key)  {
        try {
            return clientPool.borrowObject(key);
        }
        catch (InvocationTargetException e) {
//            logger.error("Failed to get client, exception: ", e);
//            return null;
            throw new DataMilvusException("Failed to get client, exception: ",e.getTargetException());
        }
        catch (Exception e) {
//            logger.error("Failed to get client, exception: ", e);
//            return null;
            throw new DataMilvusException("Failed to get client, exception: ",e);
        }
    }

PoolClientFactory create(String key) should throw the exception when exception occured:

@Override
    public T create(String key) throws Exception {
        try {
            T client = (T) constructorExt.newInstance(this.configExt);
            return client;
        } 
        catch (Exception e) {
//            return null;
            throw e;
        }
    }

[xiaofan-luan]

Agreed.

Usually what we do in java is to create exception instead of return null. if no client is there in the pool they can simply wait but if error throwed then user should error instead of null which may cause further NPE and there is no way to know the reason

[yhmo]

Similar to https://github.com/milvus-io/milvus-sdk-java/issues/1118 Will throw the exception in the next minor version.

[yhmo]

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.

[yin-bp]

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.

Good job and PoolClientFactory create method should thrown exception too：

public class PoolClientFactory<C, T> extends BaseKeyedPooledObjectFactory<String, T> {
    protected static final Logger logger = LoggerFactory.getLogger(PoolClientFactory.class);
    private final C config;
    private Constructor<?> constructor;
    private Method closeMethod;
    private Method verifyMethod;

    public PoolClientFactory(C config, String clientClassName) throws ClassNotFoundException, NoSuchMethodException {
        this.config = config;
        try {
            Class<?> clientCls = Class.forName(clientClassName);
            Class<?> configCls = Class.forName(config.getClass().getName());
            constructor = clientCls.getConstructor(configCls);
            closeMethod = clientCls.getMethod("close", long.class);
            verifyMethod = clientCls.getMethod("clientIsReady");
        } catch (Exception e) {
            logger.error("Failed to create client pool factory, exception: ", e);
            throw e;
        }
    }

    @Override
    public T create(String key) throws Exception {
        try {
            T client = (T) constructor.newInstance(this.config);
            return client;
        } catch (Exception e) {
            //return null;
            throw new MilvusClientException(ErrorCode.CLIENT_ERROR, e);
        }
    }

    @Override
    public PooledObject<T> wrap(T client) {
        return new DefaultPooledObject<>(client);
    }

    @Override
    public void destroyObject(String key, PooledObject<T> p) throws Exception {
        T client = p.getObject();
        closeMethod.invoke(client, 3L);
    }

    @Override
    public boolean validateObject(String key, PooledObject<T> p) {
        try {
            T client = p.getObject();
            return (boolean) verifyMethod.invoke(client);
        } catch (Exception e) {
            logger.error("Failed to validate client, exception: " + e);
            //return true;
            return false;
        }
    }

    @Override
    public void activateObject(String key, PooledObject<T> p) throws Exception {
        super.activateObject(key, p);
    }

    @Override
    public void passivateObject(String key, PooledObject<T> p) throws Exception {
        super.passivateObject(key, p);
    }
}

On the other way when validateObject method exception occour can return false？

[yhmo]

I don't intend to change the methods of PoolClientFactory. It derived from BaseKeyedPooledObjectFactory and implemented the virtual functions such as create/destroyObject/validateObject/activateObject. These functions are invoked by BaseKeyedPooledObjectFactory and the GenericKeyedObjectPool class. I think it is better to let the internal logic of BaseKeyedPooledObjectFactory/GenericKeyedObjectPool catch the original exceptions.

https://commons.apache.org/proper/commons-pool/apidocs/src-html/org/apache/commons/pool2/BaseKeyedPooledObjectFactory.html https://commons.apache.org/proper/commons-pool/apidocs/src-html/org/apache/commons/pool2/impl/GenericKeyedObjectPool.html

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// Return null for the create(String key) is ok, because the BaseKeyedPooledObjectFactory eventually returns a NullPointerException if the create(String key) returns null. And the NullPointerException is catched by GenericKeyedObjectPool.

079    public PooledObject<V> makeObject(final K key) throws Exception {
080        return wrap(
081                Objects.requireNonNull(create(key), () -> String.format("BaseKeyedPooledObjectFactory(%s).create(key=%s) = null", getClass().getName(), key)));
082    }

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// If the validateObject() method returns false, the GenericKeyedObjectPool will destroy the MilvusClient object.

The getObject() is a simple method directly returns the MilvusClient object, doesn't throw any exceptions. The verifyMethod is a reflected method that calls MilvusClient.clientIsReady(), and MilvusClient.clientIsReady() calls io.grpc.ManagedChannel.isShutdown()/isTerminated(), doesn't throw exceptions either. So, exception try/catch doesn't take effect here.

public boolean validateObject(String key, PooledObject<T> p) {
        try {
            T client = p.getObject();
            return (boolean) verifyMethod.invoke(client);
        } catch (Exception e) {
            logger.error("Failed to validate client, exception: " + e);
            return true;
        }
}

[yin-bp]

If create(String key) method ignore the exception ,the caller will not known the reason that cause the exception ，and null but really is milvus server is invalidate，will misleading and confusing users, so the exception should be thrown.

If an invalidate excepetion occur , destroy the invalidate object is right.

[yhmo]

This pr will throw the exception for create()/validateObject(): https://github.com/milvus-io/milvus-sdk-java/pull/1172