You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by JamesRTaylor <gi...@git.apache.org> on 2018/03/22 17:58:12 UTC
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r176506809
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java ---
@@ -2405,16 +2413,26 @@ public Void call() throws Exception {
openConnection();
hConnectionEstablished = true;
boolean isDoNotUpgradePropSet = UpgradeUtil.isNoUpgradeSet(props);
+ boolean doesSystemCatalogAlreadyExist = false;
--- End diff --
The flow of this code is very confusing. Let's do the compatibility check only once in ensureTableCreated by making the following changes:
- do not call ensureTableCreated from createTable if the table is a system table
- call ensureTableCreated instead here and remove this entire try block
- do all checks required in ensureTableCreated where we already determine if the table exists or not
- in ensureTableCreated, if SYSTEM.CATALOG doesn't exist and !isAutoUpgradeEnabled or isDoNotUpgradePropSet, then throw our standard UpgradeRequiredException immediately without creating system catalog metadata.
- the only call to checkClientServerCompatibility should be in ensureTableCreated (when isMetaTable is true)
- make sure to be defensive in the creation of the SYSTEM namespace and moving of SYSTEM tables as it's possible that multiple clients may be attempting to do that.
I think this improve the maintainability of this code (and fix this issue too).
---