Azure / azure-cosmosdb-java

Java Async SDK for SQL API of Azure Cosmos DB
MIT License
54 stars 61 forks source link

V3 store consistency layer #194

Closed kushagraThapar closed 5 years ago

kushagraThapar commented 5 years ago

V3 Spring Reactor

Major Changes


This change is Reviewable

moderakh commented 5 years ago

two points:

  1. using Exceptions.propagate(.) will cause the CosmosException to get wrapped in RuntimeException and as a result, the consistency logic which relies on the exact type of exception may not work as expected, and that introduces bugs. This is important that the exceptions don't get wrapped in RuntimeException

    ~/github/azure/azure-cosmosdb-java$ grep Exceptions.propagate -r .
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/ConsistencyReader.java:                throw Exceptions.propagate(e);
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/ConsistencyReader.java:                throw Exceptions.propagate(e);
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/ConsistencyReader.java:                        throw Exceptions.propagate(e);
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/ConsistencyReader.java:                    throw Exceptions.propagate(dce);
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/BarrierRequestHelper.java:                throw Exceptions.propagate(new InternalServerErrorException(RMResources.InternalServerError));
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                                    throw Exceptions.propagate(e);
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                                                    throw Exceptions.propagate(e);
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                                    throw Exceptions.propagate(new GoneException(RMResources.ReadQuorumNotMet));
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                                                throw Exceptions.propagate(e);
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                                            throw Exceptions.propagate(new GoneException(String.format(RMResources.ReadQuorumNotMet, readQuorumValue)));
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                                throw Exceptions.propagate(new InternalServerErrorException(RMResources.InternalServerError));
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                throw Exceptions.propagate(new GoneException());
    ./direct-impl/src/main/java/com/azure/data/cosmos/directconnectivity/QuorumReader.java:                            throw Exceptions.propagate(e);
  2. The choice of not returning null value in caches and instead returning Mono.empty() is technically changing the consistency logic, and is a cascading change to all the consumers. This has risks, another option is to return Mono<Optional> to stick to the current logic. I suggest you think about this approach unless you are sure that you have fully covered the needed changes for moving away from the nulls.

christopheranderson commented 5 years ago

/azp run

azure-pipelines[bot] commented 5 years ago
Azure Pipelines successfully started running 1 pipeline(s).
moderakh commented 5 years ago

Mono.empty() is good for us in the long run. I think it's better to take this risk early on in the stage of v3, as compared to in the long run, where we might never switch it.

What are the benefits of Mono.empty() which might be good for the long run comparing to Mono[Optional]? Could you explain please?

christopheranderson commented 5 years ago

/azp run

azure-pipelines[bot] commented 5 years ago
Azure Pipelines successfully started running 1 pipeline(s).
christopheranderson commented 5 years ago

Some tests failures, but they are all timeout issues and a session token issue that we're not surprised by.