Add gPRC layer for DistributedTransaction #216
Conversation
| mutations.add(ProtoUtil.toMutation(mutation)); | ||
| } | ||
|
|
||
| if (mutations.size() == 1) { |
There was a problem hiding this comment.
I think we can simply use mutations with the else logic without checking if there is a single element.
Creating an unnecessary list is required for put and delete but I think the overhead is negligible.
(I know the same logic exists in DistributedStorageService but I noticed it now.)
What do you think?
There was a problem hiding this comment.
Yes, I thought the overhead of creating an unnecessary list is negligible, too. My concern here is that we are not sure the performance (or overhead) difference between the single put/delete and the multiple put/delete/mutate, which depends on the storage implementation. For example, one storage implementation has an optimization for single put/delete, and the single put/delete might be faster than the multiple put/delete/mutate when we execute one mutation. That's why I have this special case for one mutation. What do you think?
There was a problem hiding this comment.
From the perspective of separation of concerns, I think an upper layer shouldn't care about the implementation or the optimization of the underlining layer if it's passing the same information, and the underlining layer should do proper optimization based on the given information.
So, in this case, the underlining layer should check if a given list contains only single item if it has an optimization for single item operation. (but the current code might not be handling such things properly and should probably be fixed)
Thought? (maybe we should discuss?)
| } catch (UnknownTransactionStatusException e) { | ||
| LOGGER.error("an unknown transaction error happened during the execution", e); | ||
| responseObserver.onError(Status.UNKNOWN.withDescription(e.getMessage()).asRuntimeException()); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
I'm wondering Errors should be caught here.
I've never caught Errors but should it rather stop the server or something if an Error occurs?
There was a problem hiding this comment.
I catch Throwable here is because whenever any Throwable is thrown, I thought we should call responseObserver.onError, otherwise the client will get stuck. And for OutOfMemoryError, we have the -XX:OnOutOfMemoryError JVM flag, and we can kill the server with it. What do you think?
There was a problem hiding this comment.
Is it true that a client gets stuck when an Error occurs?
I thought Errors will end the server and the gRPC client will be notified with some runtime exception so the same behavior will be observed without dealing with Errors explicitly in this case.
| private DistributedTransaction getTransaction(String transactionId) throws TransactionException { | ||
| ActiveTransaction activeTransaction = activeTransactions.get(transactionId); | ||
| if (activeTransaction == null) { | ||
| throw new TransactionException( |
There was a problem hiding this comment.
A question and a comment.
I think this case happens mostly in a case where different connections are used in a transaction, which only happens when a connection is closed for some reason after the transaction is started.
First of all, is this right?
If so, should the exception be treated separately?
I feel it's not an INTERNAL error.
What do you think?
There was a problem hiding this comment.
I think this case could happen due to the following reasons:
- A client specifies a wrong transaction ID
- A client specifies a transaction ID that associates with a transaction that's already closed
- A transaction associated with the specified transaction ID is already expired by the
transactionExpirationThread
An INVALID_ARGUMENT error suits the 1st and 2nd cases, but I think they are rare cases. And an INTERNAL error suits the 3rd case, and I think it most likely happens. So I decided to throw an INTERNAL error for this case. What do you think?
There was a problem hiding this comment.
@feeblefakie Thank you for reviewing this!
I left some responses for your reviews. Please check them. Thanks.
| private DistributedTransaction getTransaction(String transactionId) throws TransactionException { | ||
| ActiveTransaction activeTransaction = activeTransactions.get(transactionId); | ||
| if (activeTransaction == null) { | ||
| throw new TransactionException( |
There was a problem hiding this comment.
I think this case could happen due to the following reasons:
- A client specifies a wrong transaction ID
- A client specifies a transaction ID that associates with a transaction that's already closed
- A transaction associated with the specified transaction ID is already expired by the
transactionExpirationThread
An INVALID_ARGUMENT error suits the 1st and 2nd cases, but I think they are rare cases. And an INTERNAL error suits the 3rd case, and I think it most likely happens. So I decided to throw an INTERNAL error for this case. What do you think?
| } catch (UnknownTransactionStatusException e) { | ||
| LOGGER.error("an unknown transaction error happened during the execution", e); | ||
| responseObserver.onError(Status.UNKNOWN.withDescription(e.getMessage()).asRuntimeException()); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
I catch Throwable here is because whenever any Throwable is thrown, I thought we should call responseObserver.onError, otherwise the client will get stuck. And for OutOfMemoryError, we have the -XX:OnOutOfMemoryError JVM flag, and we can kill the server with it. What do you think?
| mutations.add(ProtoUtil.toMutation(mutation)); | ||
| } | ||
|
|
||
| if (mutations.size() == 1) { |
There was a problem hiding this comment.
Yes, I thought the overhead of creating an unnecessary list is negligible, too. My concern here is that we are not sure the performance (or overhead) difference between the single put/delete and the multiple put/delete/mutate, which depends on the storage implementation. For example, one storage implementation has an optimization for single put/delete, and the single put/delete might be faster than the multiple put/delete/mutate when we execute one mutation. That's why I have this special case for one mutation. What do you think?
| private DistributedTransaction getTransaction(String transactionId) throws TransactionException { | ||
| ActiveTransaction activeTransaction = activeTransactions.get(transactionId); | ||
| if (activeTransaction == null) { | ||
| throw new TransactionException( |
| } catch (UnknownTransactionStatusException e) { | ||
| LOGGER.error("an unknown transaction error happened during the execution", e); | ||
| responseObserver.onError(Status.UNKNOWN.withDescription(e.getMessage()).asRuntimeException()); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
Is it true that a client gets stuck when an Error occurs?
I thought Errors will end the server and the gRPC client will be notified with some runtime exception so the same behavior will be observed without dealing with Errors explicitly in this case.
| mutations.add(ProtoUtil.toMutation(mutation)); | ||
| } | ||
|
|
||
| if (mutations.size() == 1) { |
There was a problem hiding this comment.
From the perspective of separation of concerns, I think an upper layer shouldn't care about the implementation or the optimization of the underlining layer if it's passing the same information, and the underlining layer should do proper optimization based on the given information.
So, in this case, the underlining layer should check if a given list contains only single item if it has an optimization for single item operation. (but the current code might not be handling such things properly and should probably be fixed)
Thought? (maybe we should discuss?)
https://scalar-labs.atlassian.net/browse/DLT-9013
Please take a look!