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).


---

Reply via email to