vihangk1 commented on a change in pull request #1095:
URL: https://github.com/apache/hive/pull/1095#discussion_r446386133
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
##########
@@ -2368,6 +2368,36 @@ public static TableSnapshot
getTableSnapshot(Configuration conf,
validWriteIdList != null ? validWriteIdList.toString() : null);
}
+ /**
+ * This is called by Driver.java for all write operations (DDL). This
updates the latest validWriteIdList in config,
+ * so that the same can be sent from HMS Client during invocation of get_*
HMS APIs.
+ */
+ public static ValidWriteIdList updateValidWriteIdList(HiveConf conf, String
fullTableName) throws LockException {
+
+ HiveTxnManager txnMgr = SessionState.get().getTxnMgr();
+ List<String> txnTables = new ArrayList<>();
+ txnTables.add(fullTableName);
+ ValidTxnWriteIdList txnWriteIds;
+ if (conf.get(ValidTxnWriteIdList.VALID_TABLES_WRITEIDS_KEY) != null) {
Review comment:
We are not doing anything special if
conf.get(ValidTxnWriteIdList.VALID_TABLES_WRITEIDS_KEY) != null so we can just
return early at the beginning.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
##########
@@ -2368,6 +2368,36 @@ public static TableSnapshot
getTableSnapshot(Configuration conf,
validWriteIdList != null ? validWriteIdList.toString() : null);
}
+ /**
+ * This is called by Driver.java for all write operations (DDL). This
updates the latest validWriteIdList in config,
+ * so that the same can be sent from HMS Client during invocation of get_*
HMS APIs.
+ */
+ public static ValidWriteIdList updateValidWriteIdList(HiveConf conf, String
fullTableName) throws LockException {
Review comment:
This method could be rewritten to improve the readability. Return type
is not used; can be changed to void.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
##########
@@ -2368,6 +2368,36 @@ public static TableSnapshot
getTableSnapshot(Configuration conf,
validWriteIdList != null ? validWriteIdList.toString() : null);
}
+ /**
+ * This is called by Driver.java for all write operations (DDL). This
updates the latest validWriteIdList in config,
+ * so that the same can be sent from HMS Client during invocation of get_*
HMS APIs.
+ */
+ public static ValidWriteIdList updateValidWriteIdList(HiveConf conf, String
fullTableName) throws LockException {
Review comment:
Also, perhaps a better name for this method could be
addTxnWriteIdsIfNotExists(), update sounds like we are overwriting it.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/compact/AlterTableCompactDesc.java
##########
@@ -71,4 +72,17 @@ public boolean isBlocking() {
public Map<String, String> getProperties() {
return properties;
}
+
+ @Override
+ public void setWriteId(long writeId) {
+ this.writeId = writeId;
+ }
+
+ @Override public String getFullTableName() {
Review comment:
nit, formatting.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/drop/AlterTableDropPartitionDesc.java
##########
@@ -100,4 +101,20 @@ public boolean getIfPurge() {
public ReplicationSpec getReplicationSpec() {
return replicationSpec;
}
+
+ @Override
+ public void setWriteId(long writeId) {
+ this.writeId = writeId;
+ }
+
+ @Override
+ public String getFullTableName() {
+ return null;
Review comment:
intentional that this returns null?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
##########
@@ -2368,6 +2368,36 @@ public static TableSnapshot
getTableSnapshot(Configuration conf,
validWriteIdList != null ? validWriteIdList.toString() : null);
}
+ /**
+ * This is called by Driver.java for all write operations (DDL). This
updates the latest validWriteIdList in config,
+ * so that the same can be sent from HMS Client during invocation of get_*
HMS APIs.
Review comment:
I don't understand this comment. We are not using this ValidWriteIdList
in get calls in this patch right? May be rephrase it to ...the same can be sent
to HMS Client for the subsequent DDL calls.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
##########
@@ -2368,6 +2368,36 @@ public static TableSnapshot
getTableSnapshot(Configuration conf,
validWriteIdList != null ? validWriteIdList.toString() : null);
}
+ /**
+ * This is called by Driver.java for all write operations (DDL). This
updates the latest validWriteIdList in config,
+ * so that the same can be sent from HMS Client during invocation of get_*
HMS APIs.
+ */
+ public static ValidWriteIdList updateValidWriteIdList(HiveConf conf, String
fullTableName) throws LockException {
+
+ HiveTxnManager txnMgr = SessionState.get().getTxnMgr();
+ List<String> txnTables = new ArrayList<>();
+ txnTables.add(fullTableName);
+ ValidTxnWriteIdList txnWriteIds;
+ if (conf.get(ValidTxnWriteIdList.VALID_TABLES_WRITEIDS_KEY) != null) {
+ txnWriteIds = new ValidTxnWriteIdList(conf.get(
+ ValidTxnWriteIdList.VALID_TABLES_WRITEIDS_KEY));
+ } else {
+ String txnString;
+ if (conf.get(ValidTxnList.VALID_TXNS_KEY) != null) {
+ txnString = conf.get(ValidTxnList.VALID_TXNS_KEY);
+ } else {
+ ValidTxnList txnIds = txnMgr.getValidTxns();
Review comment:
are these two lines needed? It seems like lines 2385-2391 should be
changed to following since we don't want to open a transaction here right?
String txnString =
Preconditions.checkNotNull(conf.get(ValidTxnList.VALID_TXNS_KEY)).toString();
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/ColStatsProcessor.java
##########
@@ -188,9 +189,12 @@ public int persistColumnStats(Hive db, Table tbl) throws
HiveException, MetaExce
HiveTxnManager txnMgr = AcidUtils.isTransactionalTable(tbl)
? SessionState.get().getTxnMgr() : null;
if (txnMgr != null) {
- request.setValidWriteIdList(AcidUtils.getTableValidWriteIdList(conf,
- AcidUtils.getFullTableName(tbl.getDbName(),
tbl.getTableName())).toString());
request.setWriteId(txnMgr.getAllocatedTableWriteId(tbl.getDbName(),
tbl.getTableName()));
+ ValidWriteIdList writeId =
Review comment:
nit, since writeId means something else. May be rename this variable to
writeIdList
##########
File path: storage-api/src/java/org/apache/hadoop/hive/common/ValidTxnList.java
##########
@@ -31,6 +31,11 @@
*/
public static final String VALID_TXNS_KEY = "hive.txn.valid.txns";
+ /**
+ * Key used to store txn id for compactor in a
+ * {@link org.apache.hadoop.conf.Configuration} object.
+ */
+ public static final String COMPACTOR_VALID_TXNS_ID_KEY =
"hive.compactor.txn.valid.txns.id";
Review comment:
Do we read this conf key anywhere in the patch? Didn't find it.
##########
File path:
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosAcidTablesBootstrap.java
##########
@@ -193,7 +193,7 @@ public void
testAcidTablesBootstrapDuringIncrementalWithOpenTxnsTimeout() throws
// t1=5+2(insert) and t2=5+5(insert, alter add column)
Review comment:
can you update this comment on why t2=5+6 now?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/Driver.java
##########
@@ -435,8 +439,14 @@ public void releaseLocksAndCommitOrRollback(boolean
commit, HiveTxnManager txnMa
}
// If we've opened a transaction we need to commit or rollback rather than
explicitly
// releasing the locks.
- driverContext.getConf().unset(ValidTxnList.VALID_TXNS_KEY);
-
driverContext.getConf().unset(ValidTxnWriteIdList.VALID_TABLES_WRITEIDS_KEY);
+ // Unset all the keys
+ for (String key : new String[] { ValidTxnList.VALID_TXNS_KEY,
ValidTxnWriteIdList.VALID_TABLES_WRITEIDS_KEY,
+ ValidTxnList.COMPACTOR_VALID_TXNS_ID_KEY,
ValidTxnWriteIdList.COMPACTOR_VALID_TABLES_WRITEIDS_KEY }) {
Review comment:
The patch sets the COMPACTOR keys in the conf but don't see where it
reads it. Perhaps we should remove the modifications related to these keys.
##########
File path:
storage-api/src/java/org/apache/hadoop/hive/common/ValidTxnWriteIdList.java
##########
@@ -32,12 +32,18 @@
*/
public static final String VALID_TABLES_WRITEIDS_KEY =
"hive.txn.tables.valid.writeids";
+ /**
+ * Key used to store valid write id list for compactor in a
+ * {@link org.apache.hadoop.conf.Configuration} object.
+ */
+ public static final String COMPACTOR_VALID_TABLES_WRITEIDS_KEY =
"hive.compactor.txn.tables.valid.writeids";
Review comment:
Do we read this conf key anywhere in the patch? Didn't find it.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/exchange/AlterTableExchangePartitionsDesc.java
##########
@@ -53,4 +54,20 @@ public Table getDestinationTable() {
public Map<String, String> getPartitionSpecs() {
return partitionSpecs;
}
+
+ @Override
+ public void setWriteId(long writeId) {
+ this.writeId = writeId;
+ }
+
+ @Override
+ public String getFullTableName() {
+ return null;
Review comment:
intentional that this returns null?
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/add/AbstractAddPartitionAnalyzer.java
##########
@@ -130,4 +131,19 @@ protected void analyzeCommand(TableName tableName,
Map<String, String> partition
protected abstract void postProcess(TableName tableName, Table table,
AlterTableAddPartitionDesc desc,
Task<DDLWork> ddlTask) throws SemanticException;
+
+ // Equivalent to acidSinks, but for DDL operations that change data.
+ private DDLDescWithWriteId ddlDescWithWriteId;
+
+ protected void setAcidDdlDesc(DDLDescWithWriteId descWithWriteId) {
+ if (this.ddlDescWithWriteId != null) {
+ throw new IllegalStateException("ddlDescWithWriteId is already set: " +
this.ddlDescWithWriteId);
+ }
+ this.ddlDescWithWriteId = descWithWriteId;
+ }
+
+ @Override public DDLDescWithWriteId getAcidDdlDesc() {
Review comment:
can you fix the formatting? You can import the coding style formatter
into your IDE by using dev-support/eclipse-styles.xml to begin with so that
such trivial formatting changes don't happen.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/Driver.java
##########
@@ -366,13 +367,16 @@ private void acquireLocks() throws
CommandProcessorException {
driverContext.getTxnManager().getTableWriteId(t.getDbName(),
t.getTableName());
}
-
DDLDescWithWriteId acidDdlDesc =
driverContext.getPlan().getAcidDdlDesc();
boolean hasAcidDdl = acidDdlDesc != null && acidDdlDesc.mayNeedWriteId();
if (hasAcidDdl) {
String fqTableName = acidDdlDesc.getFullTableName();
final TableName tn =
HiveTableName.ofNullableWithNoDefault(fqTableName);
long writeId =
driverContext.getTxnManager().getTableWriteId(tn.getDb(), tn.getTable());
+ // This updates the latest validWriteIdList for the current table in
the config, which later will be sent
+ // by HMS Client for all get_* requests.
+ // This is done as part of HIVE-21637 ( subtask : HIVE-23573 ) to
provide cache consistency.
+ AcidUtils.updateValidWriteIdList(getConf(),
AcidUtils.getFullTableName(tn.getDb(), tn.getTable()));
Review comment:
Since this patch doesn't deal with the HMSClient changes, may be just
remove it and only keep the relevant portion of the comment.
##########
File path: streaming/src/test/org/apache/hive/streaming/TestStreaming.java
##########
@@ -964,8 +964,8 @@ private void checkDataWritten(Path partitionPath, long
minTxn, long maxTxn, int
min = pd.getMinWriteId();
}
}
- Assert.assertEquals(minTxn, min);
- Assert.assertEquals(maxTxn, max);
+ Assert.assertEquals(minTxn + 1, min);
Review comment:
I think it will be helpful to add a comment on why there is a +1 since I
not trivial to understand the test case otherwise.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/compact/AlterTableCompactDesc.java
##########
@@ -71,4 +72,17 @@ public boolean isBlocking() {
public Map<String, String> getProperties() {
return properties;
}
+
+ @Override
+ public void setWriteId(long writeId) {
+ this.writeId = writeId;
+ }
+
+ @Override public String getFullTableName() {
+ return tableName;
+ }
+
+ @Override public boolean mayNeedWriteId() {
Review comment:
nit, formatting.
##########
File path: streaming/src/test/org/apache/hive/streaming/TestStreaming.java
##########
@@ -1019,8 +1019,8 @@ private void checkDataWritten2(Path partitionPath, long
minTxn, long maxTxn, int
min = pd.getMinWriteId();
}
}
- Assert.assertEquals(minTxn, min);
- Assert.assertEquals(maxTxn, max);
+ Assert.assertEquals(minTxn + 1, min);
Review comment:
I think it will be helpful to add a comment on why there is a +1
otherwise it may not be very easy to understand just by reading the test cas.e
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/DriverContext.java
##########
@@ -214,6 +219,7 @@ public long getCompactorTxnId() {
public void setCompactorTxnId(long compactorTxnId) {
this.compactorTxnId = compactorTxnId;
+ conf.setLong(ValidTxnList.COMPACTOR_VALID_TXNS_ID_KEY, compactorTxnId);
Review comment:
if not being used may be move this out of this patch.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/DriverContext.java
##########
@@ -206,6 +208,9 @@ public ValidWriteIdList getCompactionWriteIds() {
public void setCompactionWriteIds(ValidWriteIdList compactionWriteIds) {
this.compactionWriteIds = compactionWriteIds;
+ if (compactionWriteIds != null) {
+ conf.set(ValidTxnWriteIdList.COMPACTOR_VALID_TABLES_WRITEIDS_KEY,
compactionWriteIds.toString());
Review comment:
if not being used may be move this out of this patch.
----------------------------------------------------------------
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]
