danny0405 commented on code in PR #20145:
URL: https://github.com/apache/hudi/pull/20145#discussion_r4139976613
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/HiveQueryDDLExecutor.java:
##########
@@ -181,16 +181,15 @@ private static boolean isUseStatement(String sql) {
* bound for the duration, and the thread handed back as found: it may
belong to another executor
* or to an application that embeds this sync and holds a session of its own.
*/
- private List<CommandProcessorResponse> updateHiveSQLs(List<String> sqls) {
- List<CommandProcessorResponse> responses = new ArrayList<>();
+ private void updateHiveSQLs(List<String> sqls) {
HoodieTimer timer = HoodieTimer.start();
SessionState previousSession = SessionState.get();
ClassLoader previousLoader =
Thread.currentThread().getContextClassLoader();
try {
SessionState.setCurrentSessionState(sessionState);
for (String sql : sqls) {
if (hiveDriver != null) {
- responses.add(hiveDriver.run(sql));
+ HiveDriverUtil.runOrThrow(hiveDriver, sql);
Review Comment:
Optional adjacent cleanup: could we remove the surrounding `if (hiveDriver
!= null)` from this loop? Successful construction initializes the driver, and
initialization failure throws, so normal execution can call `runOrThrow`
directly for each SQL. This removes a redundant branch and avoids silently
treating skipped SQL as success if the invariant is broken. The null check in
teardown should remain because it handles partial initialization.
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/util/HiveDriverUtil.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.hive.util;
+
+import org.apache.hudi.hive.HoodieHiveSyncException;
+
+import org.apache.hadoop.hive.ql.CommandNeedRetryException;
+import org.apache.hadoop.hive.ql.Driver;
+import org.apache.hadoop.hive.ql.processors.CommandProcessorResponse;
+
+/**
+ * Runs HiveQL through a Hive {@link Driver}.
+ */
+public final class HiveDriverUtil {
Review Comment:
Non-blocking naming suggestion: consider
`HiveStatementExecutor.executeOrThrow(driver, sql)`. This class has a specific
responsibility—executing a statement and checking Hive's response—so that name
communicates its purpose more directly than the generic `HiveDriverUtil`.
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncTool.java:
##########
@@ -403,6 +404,37 @@ public void testHiveQLSyncWithBatchingEnabled() throws
Exception {
"Incremental add via parallel HiveQL batching should sync the new
partitions");
}
+ /**
+ * Hive's Driver reports a rejected statement through its response code
rather than by throwing.
+ * A table registered with a different partition column than the one the
sync is configured with
+ * rejects every ADD PARTITION, and the sync must fail instead of recording
the commit as synced.
+ */
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testHiveQLSyncFailsWhenHiveRejectsAStatement(boolean batchingEnabled)
throws Exception {
+ hiveSyncProps.setProperty(HIVE_SYNC_MODE.key(),
HiveSyncMode.HIVEQL.name());
+ hiveSyncProps.setProperty(HIVE_SYNC_BATCHING_ENABLED.key(),
String.valueOf(batchingEnabled));
+ hiveSyncProps.setProperty(META_SYNC_PARTITION_FIELDS.key(),
HoodieRecord.PARTITION_PATH_METADATA_FIELD);
+ HiveTestUtil.createCOWTable("100", 1, true);
+ reInitHiveSyncClient();
+ reSyncHiveTable();
+ assertEquals("100",
hiveClient.getLastCommitTimeSynced(HiveTestUtil.TABLE_NAME).get());
+
+ hiveSyncProps.setProperty(META_SYNC_PARTITION_FIELDS.key(), "datestr");
+ HiveTestUtil.addCOWPartition("2050/01/01", true, true, "101");
+ reInitHiveSyncClient();
+
+ HoodieException ex = assertThrows(HoodieException.class,
this::reSyncHiveTable);
+ Throwable rejection = ex;
+ while (rejection != null && !(rejection.getMessage() != null &&
rejection.getMessage().contains("Hive rejected the statement"))) {
Review Comment:
Optional simplification: Hudi already has
`org.apache.hudi.exception.ExceptionUtil.validateErrorMsg`, which searches the
cause chain with null-safe message handling. If this integration test only
needs to establish that the rejection and partition field appear in the
exception chain, the traversal can become:
```java
assertTrue(ExceptionUtil.validateErrorMsg(ex, "Hive rejected the
statement"));
assertTrue(ExceptionUtil.validateErrorMsg(ex, "datestr"));
```
Keep the commit-marker assertion below. These checks allow the two messages
to occur on different causes; retain the current traversal if requiring both on
the same cause is intentional. The focused executor tests already inspect the
rejection's contents directly.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
