Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHOENIX-5103: Can't create/drop table using 4.14 client against 4.15 server (Addendum) #607

Conversation

ChinmaySKulkarni
Copy link
Contributor

No description provided.

@ChinmaySKulkarni
Copy link
Contributor Author

@vincentpoon @twdsilva @kadirozde @lhofhansl please review.

Bytes.toString(tenantId), Bytes.toString(schemaName),
Bytes.toString(tableName), this.accessCheckEnabled);
}
// else: the client version is old, so we cannot add a task to cleanup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the client is older than 4.15 we should just throw an exception if they try to drop a base table that has child views.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twdsilva makes sense. I will handle that as part of PHOENIX-5544. Do the other changes to support creating/dropping a table look good?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

Copy link
Contributor

@twdsilva twdsilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Bytes.toString(tenantId), Bytes.toString(schemaName),
Bytes.toString(tableName), this.accessCheckEnabled);
}
// else: the client version is old, so we cannot add a task to cleanup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

@ChinmaySKulkarni
Copy link
Contributor Author

Thanks for the review @twdsilva

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One bug (I think) and a couple nits

ViewUtil.dropChildViews(env, tenantIdBytes, schemaName, tableName);
if (!Bytes.toString(schemaName).equals(QueryConstants.SYSTEM_SCHEMA_NAME)) {
byte[] sysCatOrSysChildLink = SchemaUtil.getPhysicalTableName(
clientVersion >= MIN_SPLITTABLE_SYSTEM_CATALOG ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this would be much easier to read if you extracted the clientVersion -> table byte logic into its own method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this as part of PHOENIX-5544 (#620)

@@ -361,6 +361,11 @@ public boolean isUpgradeRequired() {
return getDelegate().isUpgradeRequired();
}

@Override
public void clearUpgradeRequired() {
getDelegate().isUpgradeRequired();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be getDelegate.clearUpgradeRequired(); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Copy-paste error. Luckily this method is never really used in our upgrade code path since we use the CQSI.clearUpgradeRequired. I will add this to my changes for PHOENIX-5544 since it works with relatively the same path and 5103 is already committed.

@ChinmaySKulkarni ChinmaySKulkarni deleted the PHOENIX-5103-drop branch March 28, 2020 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants