kgyrtkirk commented on a change in pull request #2101:
URL: https://github.com/apache/hive/pull/2101#discussion_r609530193
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -8582,7 +8582,8 @@ public GetOpenTxnsInfoResponse get_open_txns_info()
throws TException {
public OpenTxnsResponse open_txns(OpenTxnRequest rqst) throws TException {
OpenTxnsResponse response = getTxnHandler().openTxns(rqst);
List<Long> txnIds = response.getTxn_ids();
- if (txnIds != null && listeners != null && !listeners.isEmpty()) {
+ boolean isHiveReplTxn = rqst.isSetReplPolicy() && rqst.getTxn_type() ==
TxnType.DEFAULT;
Review comment:
this conditional have been copy pasted to a few places - can we move it
into a method?
##########
File path:
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
##########
@@ -1014,6 +1014,7 @@ struct OpenTxnsResponse {
struct AbortTxnRequest {
1: required i64 txnid,
2: optional string replPolicy,
+ 3: optional TxnType txn_type = TxnType.DEFAULT,
Review comment:
we seem to use camelcase in this file - can we continue to do that?
I'm just curious when a field is marked as optional - and also has a default
value ; will it still be optional?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java
##########
@@ -60,8 +60,8 @@
* @return The new transaction id
* @throws LockException if a transaction is already open.
*/
- long openTxn(Context ctx, String user, TxnType txnType) throws LockException;
+ long openTxn(Context ctx, String user, TxnType txnType, String
dbUnderReplication) throws LockException;
Review comment:
I don't feel the argument `dbUnderReplication` strongly connected to the
`openTxn` command - I wonder if it would make sense to move it inside the
`Context` object - which could enable to not change the signature; and may also
come handy later on
##########
File path:
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
##########
@@ -3712,16 +3712,17 @@ public long openTxn(String user) throws TException {
}
@Override
- public long openTxn(String user, TxnType txnType) throws TException {
- OpenTxnsResponse txns = openTxnsIntr(user, 1, null, null, txnType);
+ public long openTxn(String user, TxnType txnType, String dbUnderReplication)
throws TException {
Review comment:
are there any drawbacks of keeping the old method and not modifing every
callsite by adding a new `null` argument?
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
