Copilot commented on code in PR #2594:
URL: https://github.com/apache/phoenix/pull/2594#discussion_r3789328922
##########
phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -2007,6 +2008,19 @@ private TableDescriptor ensureTableCreated(byte[]
physicalTableName,
}
}
+ // The physical HBase table may have been left disabled by a previous
failed drop
+ // (or manual admin action) while its Phoenix metadata was removed.
Re-enable it so
+ // downstream steps (modifyTable, post-DDL SYSTEM.CATALOG RPC, and the
client's
+ // subsequent scans) see a usable table.
+ if (
+ tableType != PTableType.SYSTEM
+ && admin.isTableDisabled(TableName.valueOf(physicalTableName))
+ ) {
Review Comment:
This condition does not establish that Phoenix metadata is absent.
`createTable` calls `ensureTableCreated` before the SYSTEM.CATALOG mutation
reports `TABLE_ALREADY_EXISTS` (`MetaDataClient.java:3807-3817`), so `CREATE
TABLE [IF NOT EXISTS]` against an already registered but administratively
disabled table now re-enables it before succeeding/no-oping or throwing. That
violates the PR's metadata-orphan scope and can undo an intentional maintenance
action. Gate recovery on a confirmed metadata miss rather than only the
physical table state.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -2007,6 +2008,19 @@ private TableDescriptor ensureTableCreated(byte[]
physicalTableName,
}
}
+ // The physical HBase table may have been left disabled by a previous
failed drop
+ // (or manual admin action) while its Phoenix metadata was removed.
Re-enable it so
+ // downstream steps (modifyTable, post-DDL SYSTEM.CATALOG RPC, and the
client's
+ // subsequent scans) see a usable table.
+ if (
+ tableType != PTableType.SYSTEM
+ && admin.isTableDisabled(TableName.valueOf(physicalTableName))
+ ) {
+ LOGGER.info("Re-enabling disabled HBase table {} during CREATE
TABLE",
+ Bytes.toString(physicalTableName));
+ enableTable(admin, TableName.valueOf(physicalTableName));
Review Comment:
The new recovery path has no regression test. Please add an integration test
that creates an HBase table without Phoenix metadata, disables it, runs `CREATE
TABLE IF NOT EXISTS`, and verifies both successful metadata creation and an
enabled/queryable table. The existing-table case should also verify that a
disabled table with Phoenix metadata is not unintentionally re-enabled.
--
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]
