[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @JamesRTaylor the patch to 5.x-HBase-2.0 branch lgtm! Thanks! ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 +1. Nice work, @ChinmaySKulkarni! I'll get this committed. Might need some help with a patch that applied to 5.x-HBase-2.0 branch. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 Squashed all commits and rebased. @JamesRTaylor ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 Thanks for the updates, @ChinmaySKulkarni. Looks like you still need to rebase it. Also, one test that would be really valuable would be to: * disable auto upgrade * run execute upgrade * make some change to the HBase metadata of SYSTEM.CATALOG through ALTER TABLE or direct HBase API call (for example, set VERSIONS to some value). * connect a client that create a new ConnectionQueryServices instance to be created(see UpdateCacheAcrossDifferentClientsIT for how to do that) * ensure that the metadata change to SYSTEM.CATALOG is not undone. * do the same with auto upgrade enabled (which might cause the change to be reverted which is ok - this is more for making sure we understand this flow in both cases) This test will ensure that when this hits production, if we have auto upgrade disabled, we can make manual changes to the SYSTEM.CATALOG. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @JamesRTaylor In the current (master) implementation of upgradeSystemTables, we only create SYSMUTEX in case we catch a TableAlreadyExistsException [link](https://github.com/apache/phoenix/blob/master/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L2683) when creating SYSCAT. Why don't we create SYSMUTEX (but not acquire a lock) in case of a NewerTableAlreadyExistsException or in case SYSCAT is successfully created? I guess this is because before this JIRA: Since we only intended to use upgradeSystemTables to actually upgrade SYSTEM tables and not to create them for the first time, SYSMUTEX was already created at this point. Is my assumption correct? With this JIRA, we now also use upgradeSystemTables to create all SYSTEM tables for the first time, so we should explicitly always try to create SYSMUTEX too. How about we call createSysMutexTableIfNotExists before trying to create SYSCAT and then only acquire the upgrade mutex in case of a migration and/or upgrade? We won't need this lock before creation of other SYSTEM tables since competing clients will get TableAlreadyExistsException (consistent with current master implementation too). ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 This looks good. Couple of minor things for the tests, and then rebase it and I think we'll be good to go. Nice work, @ChinmaySKulkarni. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @JamesRTaylor addressed review comments on ensureSystemTablesMigratedToSystemNamespace. Basically, now we return an UpgradeRequiredException even in the case that we need to migrate SYSTEM tables to the SYSTEM namespace. Just doing that was not sufficient since we would have to run 'EXECUTE UPGRADE' once for the migration and once for the upgrade (if required). So, to avoid this, I am storing the SYSCAT timestamp as part of the UpgradeRequiredException and I have separated the SYSCAT schema upgrade code to another method. Now we only acquire the SYSMUTEX lock once (1. if we need to just perform an upgrade OR 2. if we need to migrate SYSTEM tables to the SYSTEM namespace and/or also perform an upgrade). Added a test for this as well. Please let me know what you think about this approach. Thanks! ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ankitsinghal commented on the issue: https://github.com/apache/phoenix/pull/295 bq. As of now, when the server gets connected to for the first time i.e. no SYSTEM tables exist, we call ensureSystemTablesMigratedToSystemNamespace, but this just creates the HBase SYSTEM namespace and returns without migrating tables since they haven't been created yet. However, later on when we do create other SYSTEM tables, we aren't migrating them to the SYSTEM namespace right away. As per the current implementation, if there is namespace mapping enabled and no SYSTEM table exists then CREATE TABLE should automatically create SYSTEM tables in SYSTEM namespace. There should not be a need of migrating them. It used to work like this, is it not the case now? bq. Now, as per the current design, whenever a second client with NS-mapping enabled connects to the server, we will migrate these SYSTEM tables to the SYSTEM namespace when ensureSystemTablesMigratedToSystemNamespace is called within init (with my PR, this will be called inside ensureTableCreated when the table is SYSTEM.CATALOG and NS-mapping is enabled, instead of in init). My question is, shouldn't we map SYSTEM tables to the SYSTEM namespace immediately after creating them? First connection will be useless, if as per the new design, SYSTEM tables are not getting created in the correct namespace. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @aertoria ping. Also, @JamesRTaylor please take a look. I've added an IT to test various cases encountered for system table creation/upgrade. Also filed [PHOENIX-4684](https://issues.apache.org/jira/browse/PHOENIX-4684). ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @aertoria fyi, please take a look since @twdsilva is out. Thanks! ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @JamesRTaylor Ok cool. I've made the changes so checks are inside _ensureTableCreated_. Please take a look now. One thing that's confusing me is that inside our _init_ method at the moment, if an upgrade is not required, we create all other SYSTEM tables, however we don't map them to the SYSTEM namespace at that point. As of now, when the server gets connected to for the first time i.e. no SYSTEM tables exist, we call _ensureSystemTablesMigratedToSystemNamespace_, but this just creates the HBase SYSTEM namespace and returns without migrating tables since they haven't been created yet. However, later on when we do create other SYSTEM tables, we aren't migrating them to the SYSTEM namespace right away. Now, as per the current design, whenever another client with NS-mapping enabled connects to the server, we will migrate these SYSTEM tables to the SYSTEM namespace when _ensureSystemTablesMigratedToSystemNamespace_ is called within _init_ (with my PR, this will be called inside _ensureTableCreated_ when the table is SYSTEM.CATALOG and NS-mapping is enabled, instead of in _init_). My question is, shouldn't we map SYSTEM tables to the SYSTEM namespace immediately after creating them? ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 I think it's ok to make these changes. Not a problem to make DO_NOT_UPGRADE public if need be. Feel free to add new arguments if necessary to ensureTableCreated. I think any special logic around SYSTEM.CATALOG or SYSTEM:CATALOG is ok as long as it's isolated. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @JamesRTaylor I'm trying to check if we can move the logic in the try block in init to _ensureTableCreated_ and there seem to be a couple of hurdles while doing so. We don't have access to the DO_NOT_UPGRADE property which is declared to be private inside UpgradeUtil.java and we can't access its getter method 'isNoUpgradeSet' from _ensureTableCreated_ because we have properties of type 'ReadOnlyProps', not 'Properties' (I couldn't find any converter methods between these instances). We need this property to decide whether or not to set _setUpgradeRequired_ to true. Also, we call _ensureSystemTablesMigratedToSystemNamespace_ inside this try block in init and also have a check which basically throws an InconsistentNamespaceMappingProperties exception if phoenix.schema.mapSystemTablesToNamespace is false and SYSTEM:CATALOG still exists. Now if we move all this logic to _ensureTableCreated_, we would have to add extra checks inside this to make sure whether _ensureTableCreated_ is called for SYSTEM.CATALOG or SYSTEM:CATALOG. Because of this, I don't think it's possible to move all this to _ensureTableCreated_. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 Thinking more, I think itâs fine to go through the CREATE TABLE code path and move the logic in the try block of init down into ensureTableCreated. Itâs only called once on the first connection made to a cluster from a client. Keeping the code simple and doing a single RPC to get the metadata and another to get the version will outweigh the minor overhead of compiling CREATE TABLE (plus like you mentioned before, we need to do that to build up the args for ensureTableCreated anyway). ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 Ok, sounds like a good plan. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user ChinmaySKulkarni commented on the issue: https://github.com/apache/phoenix/pull/295 @JamesRTaylor Oops, done. Thanks! ---
