zookeeper-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [zookeeper] szepet commented on a change in pull request #959: ZOOKEEPER-3402: Add multiRead operation
Date Sat, 15 Jun 2019 00:03:17 GMT
szepet commented on a change in pull request #959: ZOOKEEPER-3402: Add multiRead operation
URL: https://github.com/apache/zookeeper/pull/959#discussion_r294024950
 
 

 ##########
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/MultiTransactionRecord.java
 ##########
 @@ -28,20 +28,25 @@
 import java.util.List;
 
 /**
- * Encodes a composite transaction.  In the wire format, each transaction
+ * Encodes a composite operation.  In the wire format, each operation
  * consists of a single MultiHeader followed by the appropriate request.
  * Each of these MultiHeaders has a type which indicates
- * the type of the following transaction or a negative number if no more transactions
+ * the type of the following operation or a negative number if no more operations
  * are included.
+ * All of the operations must be from the same OpKind.
+ * Note: This class is called MultiTransactionRecord because of legacy reasons,
+ * MultiOperationRecord would be more accurate.
  */
 public class MultiTransactionRecord implements Record, Iterable<Op> {
 
 Review comment:
   On one hand, I definitely agree with you that the name is confusing and could be improved.
Going into implementation details, I think it would be the best to create a `MultiOperationRecord`
base (super) class which could have `MultiReadRecord` and `MultiTransactionRecord` subclasses.
   On the other hand, (besides resolving confusion caused by the name) I do not see how it
helps as to avoid any checks. It would require us to overload some function for Txn/Read operations
which would lead to more duplicated code. Another way is that we could have an `instanceof`
check at the beginning of these function instead of the `OpKind` checks but again, this would
result the some checks.
   Since `Op`, `OpResult,` and `MultiResponse` is preserved as a single class (though these
could be extended into more subclasses as well like `ReadOp/TransactionOp`, `MultiTxnResponse/MultiReadResponse`)
to maintain the current workflow, I say it is better and more consistent to preserve it as
well and make decisions based on `OpKind`.
   But of course, I am convincible on this topic so feel free to argue! :) What do you think?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message