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

Apache Phoenix Support #1012

Merged
merged 1 commit into from Dec 23, 2015
Merged

Conversation

@jmahonin
Copy link
Contributor

@jmahonin jmahonin commented May 11, 2015

Updated PR, reference issue:
#929

@jmahonin jmahonin mentioned this pull request May 11, 2015
@anirudha13
Copy link

@anirudha13 anirudha13 commented May 18, 2015

@jmahonin I got the failing unit test, MigrationTestCase#validateClean to pass by renaming the file, PhoenixCheckValidate1__First.sql to CheckValidate1__First.sql . Since the sqlMigrationPrefix is set as "CheckValidate" in the unit test.

@jmahonin
Copy link
Contributor Author

@jmahonin jmahonin commented May 18, 2015

@anirudha13 Thanks. That's likely the only change in the new PR, but I realized I pushed the last one in error. Can you verify the new one if you have a chance?

@anirudha13
Copy link

@anirudha13 anirudha13 commented May 18, 2015

@jmahonin
Attaching a diff with the changes that I made to make sure that unit test passed, along with the other unit tests as well. Just look at the changes to files, flyway-core/src/test/java/org/flywaydb/core/migration/MigrationTestCase.java and flyway-core/src/test/java/org/flywaydb/core/internal/dbsupport/phoenix/PhoenixMigrationMediumTest.java

diff --git a/flyway-core/src/test/java/org/flywaydb/core/internal/dbsupport/phoenix/PhoenixMigrationMediumTest.java b/flyway-core/src/test/java/org/flywaydb/core/internal/db
index 7be4e16..9cce0d3 100644
--- a/flyway-core/src/test/java/org/flywaydb/core/internal/dbsupport/phoenix/PhoenixMigrationMediumTest.java
+++ b/flyway-core/src/test/java/org/flywaydb/core/internal/dbsupport/phoenix/PhoenixMigrationMediumTest.java
@@ -52,11 +52,6 @@ public class PhoenixMigrationMediumTest extends MigrationTestCase {
         return "migration/dbsupport/phoenix/sql/quote";
     }

-    @Override
-    protected String getCheckValidateSqlPrefix() {
-        return "PhoenixCheckValidate";
-    }
-
     @BeforeClass
     public static void beforeClassSetUp() throws Exception {
         // Startup HBase in-memory cluster
@@ -215,4 +210,4 @@ public class PhoenixMigrationMediumTest extends MigrationTestCase {
     public void setCurrentSchema() throws Exception {}


-}
+}
\ No newline at end of file
diff --git a/flyway-core/src/test/java/org/flywaydb/core/migration/MigrationTestCase.java b/flyway-core/src/test/java/org/flywaydb/core/migration/MigrationTestCase.java
index 5d41d45..500e3b5 100644
--- a/flyway-core/src/test/java/org/flywaydb/core/migration/MigrationTestCase.java
+++ b/flyway-core/src/test/java/org/flywaydb/core/migration/MigrationTestCase.java
@@ -263,14 +263,10 @@ public abstract class MigrationTestCase {

         flyway.setValidateOnMigrate(true);
         flyway.setCleanOnValidationError(true);
-        flyway.setSqlMigrationPrefix(getCheckValidateSqlPrefix());
+        flyway.setSqlMigrationPrefix("CheckValidate");
         assertEquals(1, flyway.migrate());
     }

-    protected String getCheckValidateSqlPrefix() {
-        return "CheckValidate";
-    }
-
     @Test
     public void failedMigration() throws Exception {
         String tableName = "before_the_error";
@nycjay
Copy link

@nycjay nycjay commented Jul 14, 2015

Any idea when Phoenix support will make it into a Flyway release?

@jmahonin jmahonin force-pushed the jmahonin:phoenix_support_latest branch 2 times, most recently from ea2e7b1 to fae2bc4 Jul 15, 2015
@jmahonin
Copy link
Contributor Author

@jmahonin jmahonin commented Jul 15, 2015

I've just re-upped this PR with a new version that fixes unit tests, and uses an updated Phoenix / HBase version for the tests as well.

@anirudha13 @nycjay If you guys can give this branch a test drive and see if it works for you, that'd be awesome.

@axelfontaine Please let me know if you'd like any changes made

@nycjay
Copy link

@nycjay nycjay commented Jul 15, 2015

@jmahonin thanks. still have a bit of setup work to do on our end, just getting started with phoenix. will reply back when we a chance to test, but it may be a couple of weeks.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Oct 27, 2015

Any updates on this?

@jmahonin
Copy link
Contributor Author

@jmahonin jmahonin commented Oct 27, 2015

We've been using this in production for months without issue, and the PR has been ready to go for months. It looks like there's some new merge conflicts that we need to resolve though. Do you have any specific comments with the PR?

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Oct 28, 2015

Thanks. Could you make it mergeable again?

Also, a few things seem problematic:

  • why can't Phoenix tests be run by default?
  • why did you need to make changes to MetaDataTableImpl? Is it only for the update checksum stuff? Doesn't the existing syntax work?
  • why the changes in MigrationTestCase?
  • why did you duplicate all sql files in src/test/resources/migration/dbsupport?

Cheers
Axel

@jmahonin
Copy link
Contributor Author

@jmahonin jmahonin commented Oct 29, 2015

I'll work on the merge issues.

Issue responses:

  1. I think previously the Phoenix tests were being run in the 'test' phase, but setting up the in-memory HBase cluster is a bit slow and computationally expensive, so I had them disabled by default. It appears now they are running in the 'verify' phase, so running by default sounds good to me, if you'd prefer that.

  2. The MetaDataTableImpl changes are specifically to support Phoenix syntax. It doesn't support UPDATE, only UPSERT. All of the changes in the core files are to support loading a modified syntax.

  3. Most of the changes in the MigrationTestCase don't appear to be needed anymore, now that there are overridable paths, however I have a caveat in the last paragraph.

  4. None of the dbsupport files are "duplicated", they're all copied and modified to work with Phoenix syntax. If I recall, mostly it was things like replacing UPDATE with UPSERT, modifying PRIMARY KEY syntax, and changing data types where needed (INT -> INTEGER).

While I have your attention though, on the first iteration of this PR, I was getting a "SCHEMA" applied migration type for the first entry of the migration table, and then subsequent "SQL" types after that for the real migrations. If I recall, it's triggered by coding around the fact that a schema doesn't exist in Phoenix until a table is created. Everything continued to work properly with this "SCHEMA" entry, although I had to override several of the unit tests that assumed the first entry was 'SQL'.

I'm curious if this "SCHEMA" migration type is expected behaviour, or is this something that I should try and avoid having completely? I couldn't really work out the reasoning behind if from the code and comments alone.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Oct 29, 2015

  1. db tests are being run in the "integration-test" phase
  2. maybe this should be abstracted by dbsupport instead
  3. ok
  4. ok

SCHEMA is used when Flyway creates the schema. It is absent when the schema already existed before Flyway ran the first time.

@jmahonin jmahonin force-pushed the jmahonin:phoenix_support_latest branch 2 times, most recently from 277a501 to 9d00764 Nov 3, 2015
It has been set up as an EmbeddedDB, although the Phoenix tests
are disabled by default unless run in the new 'PhoenixDBTest'
profile via:

mvn test -P PhoenixDBTest -P-CommercialDBTest -P-InstallableDBTest

Notable changes to core classes:

* MetaDataTableImpl:

Due to Phoenix's peculiar syntax, the hardcoded statements in
addAppliedMigration() and updateChecksum() won't work. It now
attempts to load a .sql template and run that, if it exists.

* MigrationTestCase:

Added checks for applied migrations of type MigrationType.SCHEMA
and adjusted asserts accordingly. Also added 'getBaseDir()' and
'getMigrationDir()' methods and replaced direct calls to BASEDIR
or other hardcode paths with them. Several of the tests in
MigrationTestCase can then be used in the Phoenix tests by
implementing those method. There are, however, a few cases due
to incompatible SQL, or other Phoenix quirks, where specific
tests are overridden.
@jmahonin jmahonin force-pushed the jmahonin:phoenix_support_latest branch from 9d00764 to e13f8e4 Nov 3, 2015
@jmahonin
Copy link
Contributor Author

@jmahonin jmahonin commented Nov 3, 2015

Back to mergeable.

Re: 1, I've left it in its own profile. The integration tests take around 5m to run, but if you'd like them in the EmbeddedDBTest profile, let me know

Re: 2, I can try roll that into this patch if you'd prefer as well.

@ndimiduk
Copy link

@ndimiduk ndimiduk commented Nov 16, 2015

Bump. Any activity here? I'd love to make use of this.

@axelfontaine axelfontaine added this to the Flyway 4.0 milestone Dec 23, 2015
@axelfontaine axelfontaine merged commit e13f8e4 into flyway:master Dec 23, 2015
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Dec 23, 2015

Thank you Josh for all the work you put into this! Merged.

@jmahonin
Copy link
Contributor Author

@jmahonin jmahonin commented Dec 23, 2015

Thanks @axelfontaine !

Let me know if there's any further cleanup work you like done. It looks like you've just merged the docs branch in as well, I'll take a once-over to see if there's anything changed there as well, but I suspect it's fine.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Dec 23, 2015

Is there a single jar (no transitive deps) for the jdbc driver that could be shipped out of the box with the Flyway command-line tool?

@jmahonin
Copy link
Contributor Author

@jmahonin jmahonin commented Dec 23, 2015

There is an assembly client JAR which is self-contained, but not published to maven, available from the Phoenix downloads page [1].

Unfortunately, the client JAR must always be at least the same version or newer than the corresponding server JAR, and to add to the complexity, there are three supported versions of HBase, each with their own client JAR.

As well, the Phoenix release cycle is pretty active, so it may be difficult to ship up-to-date versions. Would it be possible instead to package installation instructions specifically for the Phoenix JAR? Most Phoenix users are pretty accustomed to grabbing and moving around the JAR files as necessary anyway.

[1] https://phoenix.apache.org/download.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.