phoenix icon indicating copy to clipboard operation
phoenix copied to clipboard

Phoenix 6653 Add upgrade tests based on HBase snapshots

Open richardantal opened this issue 2 years ago • 2 comments

Work is still in progress

richardantal avatar Aug 08 '22 07:08 richardantal

There is a separate PR for pre 4.10 upgrade https://github.com/apache/phoenix/pull/1474 But I'd like to trigger a test run here with these 2. I will rebase Phoenix 6653 to the master.

richardantal avatar Aug 08 '22 07:08 richardantal

(We've had offline discussions about this with Richard, so some context may not be immediately apparent from the patch)

You have uncovered two different additional problems here.

One is that moveChildLinkConnection doesn't handle namespace mapping. Your current solution here is incorrect, because it manipulates the pre-upgrade default namespace system tables, even though by this point it has already been moved to the NS (and upgraded). The correct solution would be to replace both TableName.valueOf(SYSTEM_CATALOG_NAME) and TableName.valueOf(SYSTEM_CHILD_LINK_NAME) with the right namespace aware method from SchmaUtil which preforms the '.' -> ':' replacement as needed. (it is also much simpler)

This bugfix may even be split into a separate dependent ticket, if you prefer. We should also check if we have other similar bugs in the upgrade code.

The other problem is that moveChildLinkConnection creates a the HBase Connection from a vanilla Configuration object (reading from hbase-site.xml), which doesn't get necessary Config changes that were set by BaseTest/minicluster.

Simply copying the ZK quorum from the CQSI connection may get the test to pass, but is not the right way. We should clone the Configuration object from CQSI, apply the necessary the changes to the clone, and use that for creating the HBaseConnection.

stoty avatar Aug 11 '22 16:08 stoty

Thank you @stoty for the review and the suggestions, I've updated the log msgs, and added more detail to the commit message.

richardantal avatar Aug 15 '22 11:08 richardantal

Thank you @stoty for the reviews, I close this PR and merge the other 2 separately.

richardantal avatar Aug 16 '22 07:08 richardantal