Phoenix 6653 Add upgrade tests based on HBase snapshots by richardantal · Pull Request #1480 · apache/phoenix

richardantal · 2022-08-08T07:51:56Z

Work is still in progress

richardantal · 2022-08-08T07:54:56Z

There is a separate PR for pre 4.10 upgrade #1474
But I'd like to trigger a test run here with these 2. I will rebase Phoenix 6653 to the master.

stoty

You should either duplicate the template class, and put the test there, or rename it.
Also, I think that instead of committing the snapshot directories directly, storing them as a single zip file, or in a zip file per snapshot, and extracting in the test would be better.
The individial files are not human reabable anyway.

stoty · 2022-08-11T16:02:21Z

(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 · 2022-08-11T16:06:15Z

phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java


        Configuration conf = HBaseConfiguration.create();
+
+        Configuration oldconf = HBaseConfiguration.create(config);


Simply use
Configuration conf = HBaseConfiguration.create(config);
and you'll automatically get all properties, including ZK.

stoty · 2022-08-11T16:22:49Z

phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java

+     * @throws SQLException
+     */
+
+    public static void moveOrCopyChildLinks(PhoenixConnection oldMetaConnection, Map<String, String> options) throws IOException {


This is the wrong workaround.

stoty · 2022-08-15T10:35:29Z

phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java

+        TableName sysCat = SchemaUtil.getPhysicalTableName(SYSTEM_CATALOG_NAME, readOnlyProps);
+        TableName sysChildLink = SchemaUtil.getPhysicalTableName(SYSTEM_CHILD_LINK_NAME, readOnlyProps);
+
+        LOGGER.info(String.format("SYSTEM CATALOG tabled use for copying child links: %s", sysCat.toString()));


Once the testing is done, you may want remove this, or change it to DEBUG level.

stoty

Please also mention the addParentToChildLinks namespace fix in the commit message.

stoty · 2022-08-15T11:35:49Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/LoadSystemTableSnapshotIT.java

+        //Now we can start Phoenix
+        setUpTestDriver(new ReadOnlyProps(serverProps.entrySet().iterator()), new ReadOnlyProps(clientProps.entrySet()
+                .iterator()));
        assertTrue(true);


Is this assertion enough ?
Will the driver throw an exception if there is an upgrade problem ?

I think, yes
If you see the tests results in my other PR, without the PHOENIX-6754 fix, that fails with
org.apache.phoenix.schema.ColumnNotFoundException: ERROR 504 (42703): Undefined column. columnName=SYSTEM.CATALOG.COLUMN_QUALIFIER
Test run on this one is successful.

Will update this and the other PR, based on the things you suggested but it would be better to merge PHOENIX-6754 before

That's fine, ping me when the PHOENIX-6754 changes are finished.

PHOENIX-6754 is finished, but we wanted to run these tests on the change
#1474

richardantal · 2022-08-15T11:56:39Z

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

stoty · 2022-08-15T13:01:23Z

phoenix-core/pom.xml

      <groupId>org.apache.commons</groupId>
      <artifactId>commons-csv</artifactId>
    </dependency>
+    <dependency>


This should be test scope.

Corrected the HBase table names based on namespace mapping in moveOrCopyChildLinks during upgrade

stoty

+1 LGTM
but wait for the tests.

richardantal · 2022-08-16T07:25:22Z

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

PHOENIX-6754 Upgrades from pre 4.10 versions are broken again

2e6b4df

richardantal force-pushed the PHOENIX-6653 branch from cbdc45f to 7f824b9 Compare August 8, 2022 07:54

richardantal force-pushed the PHOENIX-6653 branch from 7f824b9 to 6ab22f0 Compare August 8, 2022 14:53

stoty requested changes Aug 9, 2022

View reviewed changes

richardantal force-pushed the PHOENIX-6653 branch 2 times, most recently from ed33be3 to 183dd8c Compare August 10, 2022 16:03

stoty reviewed Aug 11, 2022

View reviewed changes

stoty requested changes Aug 11, 2022

View reviewed changes

richardantal force-pushed the PHOENIX-6653 branch from 183dd8c to 3788e66 Compare August 15, 2022 10:17

stoty reviewed Aug 15, 2022

View reviewed changes

stoty requested changes Aug 15, 2022

View reviewed changes

stoty reviewed Aug 15, 2022

View reviewed changes

richardantal force-pushed the PHOENIX-6653 branch from 3788e66 to db9f50f Compare August 15, 2022 11:54

richardantal force-pushed the PHOENIX-6653 branch from db9f50f to 629149d Compare August 15, 2022 12:16

stoty reviewed Aug 15, 2022

View reviewed changes

PHOENIX-6653 Add upgrade tests based on HBase snapshots

86d13a0

Corrected the HBase table names based on namespace mapping in moveOrCopyChildLinks during upgrade

richardantal force-pushed the PHOENIX-6653 branch from 629149d to 86d13a0 Compare August 15, 2022 13:13

richardantal requested a review from stoty August 15, 2022 14:24

stoty approved these changes Aug 15, 2022

View reviewed changes

richardantal mentioned this pull request Aug 15, 2022

PHOENIX-6653 Add upgrade tests based on HBase snapshots #1485

Merged

richardantal closed this Aug 16, 2022


		Configuration conf = HBaseConfiguration.create();

		Configuration oldconf = HBaseConfiguration.create(config);

Conversation

richardantal commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardantal commented Aug 8, 2022

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

stoty commented Aug 11, 2022

Uh oh!

stoty Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

stoty Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

stoty Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

stoty Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

richardantal Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

stoty Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardantal Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

richardantal commented Aug 15, 2022

Uh oh!

stoty Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

richardantal commented Aug 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

richardantal commented Aug 8, 2022 •

edited

Loading

stoty Aug 15, 2022 •

edited

Loading