Copilot commented on code in PR #6887:
URL: https://github.com/apache/hive/pull/6887#discussion_r4331915091
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -119,6 +132,32 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
return result;
}
+ private TableInfo getTable(long tableId) {
+ MTable table = pm.getObjectById(MTable.class, new MTable.PK(tableId));
+ if (table == null) {
+ return null;
+ }
+ Map<String, String> params = table.getParameters() == null ?
+ new HashMap<>() : table.getParameters();
+ // This table has been processed early
+ if (params.containsKey(COLUMN_DEDUPLICATOR_MARKER)) {
+ if (clearMarker) {
+ params.remove(COLUMN_DEDUPLICATOR_MARKER);
+ table.setParameters(params);
+ }
+ return null;
+ }
+
+ if (!isDryRun) {
+ params.put(COLUMN_DEDUPLICATOR_MARKER, "" + System.currentTimeMillis() /
1000);
+ table.setParameters(params);
+ }
Review Comment:
`-clearMarker` is described as clearing an existing marker, but when
`clearMarker == true` and a table *doesn’t* already have the marker, this code
will still add the marker (and then proceed to deduplicate). That can
unintentionally mark previously-unprocessed tables as processed, causing future
runs to skip them. Fix by making `clearMarker` a true “cleanup-only” mode: if
`clearMarker` is set, do not add markers (and ideally do not deduplicate); only
remove the marker when present and otherwise no-op for that table.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -56,43 +57,55 @@
* the same CD, reducing metadata bloat that can accumulate during replication.
*/
public final class ColumnDeduplicator {
+ public static final String COLUMN_DEDUPLICATOR_MARKER =
"__metatool_column_deduplication__";
private final RawStore store;
private final PersistenceManager pm;
private final AtomicReference<String> progress;
private final boolean isDryRun;
private final boolean isVerbose;
+ private final boolean clearMarker;
public ColumnDeduplicator(RawStoreBundle bundle, AtomicReference<String>
progress,
- boolean isDryRun, boolean isVerbose) {
+ boolean isDryRun, boolean isVerbose, boolean clearMarker) {
this.store = bundle.getBaseStore();
this.pm = bundle.getPersistentManager();
this.progress = progress;
this.isDryRun = isDryRun;
this.isVerbose = isVerbose;
+ this.clearMarker = clearMarker;
}
MetaToolObjectStore.DedupColumnsResult run(String catalogFilter, String
dbFilter, String tableFilter) {
- List<TableInfo> tables = findPartitionedTables(catalogFilter, dbFilter,
tableFilter);
+ List<Long> tables = findPartitionedTables(catalogFilter, dbFilter,
tableFilter);
MetaToolObjectStore.DedupColumnsResult result = new
MetaToolObjectStore.DedupColumnsResult(tables.size());
long start = System.currentTimeMillis();
for (int i = 0; i < tables.size() && result.getException() == null; i++) {
boolean committed = false;
- TableInfo table = tables.get(i);
+ long tableId = tables.get(i);
PostCommitValidation validation = null;
+ TableInfo table = null;
store.openTransaction();
try {
Deadline.checkTimeout();
- validation = deduplicateTable(table, result);
+ table = getTable(tableId);
+ // This target table might be dropped or deduplicated, ignore
+ // If this is the stage to clean up the marker, ignore
+ if (table != null) {
+ validation = deduplicateTable(table, result);
+ }
committed = store.commitTransaction();
- } catch (Exception ex) {
+ } catch (Throwable ex) {
+ System.err.println("Failed to deduplicate the column in this table: "
+ (table == null ? tableId : table));
+ ex.printStackTrace(System.err);
result.catchException(ex);
Review Comment:
Catching `Throwable` will also catch serious `Error`s (e.g.,
`OutOfMemoryError`, `StackOverflowError`) that typically should not be
swallowed and converted into a normal tool failure. Prefer catching `Exception`
(or a narrower set) here; if you must catch `Throwable` for logging, rethrow
when `ex instanceof Error` after logging so the process can fail fast in
unrecoverable situations.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/MetaToolObjectStore.java:
##########
@@ -977,7 +977,7 @@ public static final class DedupColumnsResult {
private int columnDescriptorsRemoved;
private int tablesValidated;
private final List<String> details = new ArrayList<>();
- private Exception exception;
+ private Throwable exception;
Review Comment:
Changing `DedupColumnsResult` to store/return `Throwable` broadens the
contract to include `Error`s, which generally aren’t meant to be treated as
reportable/handled outcomes. Consider keeping the result API typed to
`Exception` (or a project-specific checked/unchecked exception type) and handle
`Error`s separately (log + rethrow) to avoid encouraging callers to treat
JVM-fatal conditions as routine results.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/MetaToolObjectStore.java:
##########
@@ -1025,18 +1025,18 @@ void addDetail(String detail) {
details.add(detail);
}
- void catchException(Exception exception) {
+ void catchException(Throwable exception) {
this.exception = exception;
}
- public Exception getException() {
+ public Throwable getException() {
return exception;
}
Review Comment:
Changing `DedupColumnsResult` to store/return `Throwable` broadens the
contract to include `Error`s, which generally aren’t meant to be treated as
reportable/handled outcomes. Consider keeping the result API typed to
`Exception` (or a project-specific checked/unchecked exception type) and handle
`Error`s separately (log + rethrow) to avoid encouraging callers to treat
JVM-fatal conditions as routine results.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -119,6 +132,32 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
return result;
}
+ private TableInfo getTable(long tableId) {
+ MTable table = pm.getObjectById(MTable.class, new MTable.PK(tableId));
+ if (table == null) {
+ return null;
+ }
Review Comment:
`PersistenceManager.getObjectById(...)` commonly throws (e.g.,
`JDOObjectNotFoundException`) when the row doesn’t exist rather than returning
`null`. Since the intent is to ignore dropped tables, this should explicitly
handle the not-found case (catch the specific exception and return null, or use
an API variant that doesn’t throw) to avoid aborting the whole run when a table
disappears mid-scan.
--
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]
