ayushtkn commented on code in PR #6887:
URL: https://github.com/apache/hive/pull/6887#discussion_r4445034089
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/MetaToolTaskDedupColumns.java:
##########
@@ -69,10 +70,10 @@ void execute() {
daemon.setDaemon(true);
daemon.start();
}
- result = getObjectStore().dedupColumns(catalogFilter, dbFilter,
tableFilter, progress, isDryRun, isVerbose);
- } catch (MetaException ex) {
- // Deadline.startTimer throws MetaException, this shouldn't happen as
the Deadline has been registered.
- System.err.println(ex.getMessage());
+ result = getObjectStore().dedupColumns(catalogFilter, dbFilter,
tableFilter, progress, isDryRun, isVerbose,
+ getCl().isClearMarker());
+ } catch (Throwable ex) {
Review Comment:
If `dedupColumns(...)` throws before returning, result stays `null`, this
catch only prints to `stderr`, and the task exits without rethrowing —
`HiveMetaTool.main` may return `0`. Consider catch (Exception ex) { ... throw
new IllegalStateException(..., ex); } (or rethrow) so failures still fail the
tool. result.getException() only covers errors recorded inside
ColumnDeduplicator.run().
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -119,6 +132,39 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
return result;
}
+ public TableInfo getTable(long tableId) {
+ final MTable table;
+ try {
+ table = pm.getObjectById(MTable.class, new MTable.PK(tableId));
+ } catch (JDOObjectNotFoundException e) {
+ // This table might have been dropped since the beginning
+ return null;
+ }
+ Map<String, String> params = table.getParameters() == null ?
+ new HashMap<>() : table.getParameters();
+ if (clearMarker) {
+ if (!params.containsKey(COLUMN_DEDUPLICATOR_MARKER)) {
+ return null;
+ }
+ params.remove(COLUMN_DEDUPLICATOR_MARKER);
Review Comment:
`-dryRun` does not guard the `-clearMarker` path: marker removal is
committed even with `-dryRun`.
Please either reject `-clearMarker` with `-dryRun` in
`HiveMetaToolCommandLine`, or skip/remove marker updates when `isDryRun` is
`true`.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/HiveMetaToolCommandLine.java:
##########
@@ -93,7 +93,8 @@ class HiveMetaToolCommandLine {
.hasOptionalArgs(3)
.withDescription("De-duplicate column descriptors for partitioned tables
to reduce metadata bloat. "
+ "Optional catalog, database, and table filters support *
wildcards. Use -dryRun to preview changes, "
- + "-verbose to print per-partition details, and -timeout to set a
timeout in seconds. "
+ + "-verbose to print per-partition details, -clearMarker to drop the
marker that has been added in previous runs, "
+ + "and -timeout to set a timeout in seconds. "
+ "Live runs validate committed state (partition column schemas
unchanged, canonical CD IDs applied) "
Review Comment:
Help describes -clearMarker but not that live runs add
__metatool_column_deduplication__ and that marked tables are skipped on re-run.
A short sentence would help operators.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -119,6 +132,39 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
return result;
}
+ public TableInfo getTable(long tableId) {
+ final MTable table;
+ try {
+ table = pm.getObjectById(MTable.class, new MTable.PK(tableId));
+ } catch (JDOObjectNotFoundException e) {
+ // This table might have been dropped since the beginning
+ return null;
+ }
+ Map<String, String> params = table.getParameters() == null ?
+ new HashMap<>() : table.getParameters();
+ if (clearMarker) {
+ if (!params.containsKey(COLUMN_DEDUPLICATOR_MARKER)) {
+ return null;
+ }
+ params.remove(COLUMN_DEDUPLICATOR_MARKER);
+ table.setParameters(params);
+ return null;
+ }
+ if (params.containsKey(COLUMN_DEDUPLICATOR_MARKER)) {
+ return null;
+ }
+
+ if (!isDryRun) {
+ params.put(COLUMN_DEDUPLICATOR_MARKER, "" + System.currentTimeMillis() /
1000);
+ table.setParameters(params);
Review Comment:
The marker is set in getTable() before deduplicateTable() runs, so live runs
mark tables even when there are no duplicate CDs (or no work). Re-runs skip
those tables until -clearMarker. If that’s intentional for resume, please say
so in the PR description; otherwise consider setting the marker only after
successful dedup (or after post-commit validation).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
