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

Support compatibility test for JDBC driver #56722

Closed
mark-vieira opened this issue May 13, 2020 · 23 comments
Closed

Support compatibility test for JDBC driver #56722

mark-vieira opened this issue May 13, 2020 · 23 comments
Assignees
Labels
:Analytics/SQL SQL querying :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team Team:QL (Deprecated) Meta label for query languages team

Comments

@mark-vieira
Copy link
Contributor

Forwards compatibility was added to our JDBC driver starting in version 7.7.0 (see #56223). The compatibility guarantee being that for any given version of the driver, it should support (mostly) communication for future versions of Elasticsearch of the same major version. For example, version 7.7.0 of the driver should be able to talk to Elasticearch nodes of version 7.7.1, 7.8.0, 7.8.1, 7.9.0 and so on...

Currently, there is no automated testing in place to ensure we maintain forwards compatibility. It's also not completely clear where, or how these tests should run. Currently, the build only supports backwards compatibility. That is, testing branch code against assets or artifacts from earlier versions or branches. Building or resolving artifacts to test against from future branches currently isn't supported, since, for example, the 7.x branch has no knowledge of the code in master.

We'll need to sort out a few things here, such as which direction we test in. Do we do backwards testing, where in which future branches grab the older JDBC drivers and run the tests, or forwards testing, where older branches grab the new Elasticsearch distribution and run the tests? Much of this is implementation detail, but it has some semantic impact, as with the former method, a failure in a given branch generally points to an incompatible change made in a different branch (but not necessarily). Going the latter route would require more effort, since we'd need to add support to build unreleased artifacts from future branches, which we currently don't do.

cc @matriv

@mark-vieira mark-vieira added :Delivery/Build Build or test infrastructure :Analytics/SQL SQL querying labels May 13, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Build)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/SQL)

@elasticmachine elasticmachine added Team:Core/Infra Meta label for core/infra team Team:QL (Deprecated) Meta label for query languages team labels May 13, 2020
@mark-vieira
Copy link
Contributor Author

It's possible that this somewhat overlaps with what @jakelandis is doing with REST compatibility, as this is another type of forwards compatibility, although implemented a bit differently. Again, in that case (correct me if I'm wrong) newer branches grab the artifact (in this case the rest specs) from the older branch and run them against the current version.

@rjernst
Copy link
Member

rjernst commented May 13, 2020

We'll need to sort out a few things here, such as which direction we test in. Do we do backwards testing, where in which future branches grab the older JDBC drivers and run the tests, or forwards testing, where older branches grab the new Elasticsearch distribution and run the tests?

I think we should continue with backwards compatibility testing, for a few reasons:

  • The compatibility layer is implemented in the "future" version. ie in 7.x, we have the compatibility layer with earlier versions, ie backwards compatibility. Thus that is where 7.x should be tested against all the versions it is compatible with.
  • As you say, backcompat is the model of tests that we already have. We already have build infrastructure for iterating over compatible versions to build tests against each version.
  • It fits better with our branching model. We rarely commit directly to release branches, but instead go through our unreleased branches. It is better to catch compatibility problems in the initial commit rather than only once we backport to a release branch.

@matriv
Copy link
Contributor

matriv commented May 18, 2020

FYI I've opened: #56872, to "isolate" the jdbc integration tests and avoid hidden bugs because of possible unnecessary pulled in dependencies, and to prepare for the support compatibility testing.

@matriv
Copy link
Contributor

matriv commented May 18, 2020

I also agree with keeping the bwc scheme. So for example: for 7.8 we can check for each 7.7.x and 7.8.x released versions of ES nodes with the [7.7.0 -> es node version - 1].

We'd like to have something soon as new features are blocked by this (for example date_nanos support PR awaits on this).

matriv added a commit that referenced this issue May 22, 2020
Move the JDBC functionality integration tests from `:sql:qa` to a separate
module `:sql:qa:jdbc`. This way the tests are isolated from the rest of the 
integration tests and they only depend to the `:sql:jdbc` module, thus 
removing the danger of accidentally pulling in some dependency that may
hide bugs.

Moreover this is a preparation for #56722, so that we can run those tests
between different JDBC and ES node versions and ensure forward
compatibility.

Move the rest of existing tests inside a new `:sql:qa:server` project, so that
the `:sql:qa` becomes the parent project for both and one can run all the integration
tests by using this parent project.
matriv added a commit to matriv/elasticsearch that referenced this issue May 22, 2020
Move the JDBC functionality integration tests from `:sql:qa` to a separate
module `:sql:qa:jdbc`. This way the tests are isolated from the rest of the
integration tests and they only depend to the `:sql:jdbc` module, thus
removing the danger of accidentally pulling in some dependency that may
hide bugs.

Moreover this is a preparation for elastic#56722, so that we can run those tests
between different JDBC and ES node versions and ensure forward
compatibility.

Move the rest of existing tests inside a new `:sql:qa:server` project, so that
the `:sql:qa` becomes the parent project for both and one can run all the integration
tests by using this parent project.

(cherry picked from commit c09f4a0)
matriv added a commit to matriv/elasticsearch that referenced this issue May 22, 2020
Move the JDBC functionality integration tests from `:sql:qa` to a separate
module `:sql:qa:jdbc`. This way the tests are isolated from the rest of the
integration tests and they only depend to the `:sql:jdbc` module, thus
removing the danger of accidentally pulling in some dependency that may
hide bugs.

Moreover this is a preparation for elastic#56722, so that we can run those tests
between different JDBC and ES node versions and ensure forward
compatibility.

Move the rest of existing tests inside a new `:sql:qa:server` project, so that
the `:sql:qa` becomes the parent project for both and one can run all the integration
tests by using this parent project.

(cherry picked from commit c09f4a0)
matriv added a commit that referenced this issue May 22, 2020
Move the JDBC functionality integration tests from `:sql:qa` to a separate
module `:sql:qa:jdbc`. This way the tests are isolated from the rest of the
integration tests and they only depend to the `:sql:jdbc` module, thus
removing the danger of accidentally pulling in some dependency that may
hide bugs.

Moreover this is a preparation for #56722, so that we can run those tests
between different JDBC and ES node versions and ensure forward
compatibility.

Move the rest of existing tests inside a new `:sql:qa:server` project, so that
the `:sql:qa` becomes the parent project for both and one can run all the integration
tests by using this parent project.

(cherry picked from commit c09f4a0)
matriv added a commit to matriv/elasticsearch that referenced this issue May 22, 2020
Move the JDBC functionality integration tests from `:sql:qa` to a separate
module `:sql:qa:jdbc`. This way the tests are isolated from the rest of the
integration tests and they only depend to the `:sql:jdbc` module, thus
removing the danger of accidentally pulling in some dependency that may
hide bugs.

Moreover this is a preparation for elastic#56722, so that we can run those tests
between different JDBC and ES node versions and ensure forward
compatibility.

Move the rest of existing tests inside a new `:sql:qa:server` project, so that
the `:sql:qa` becomes the parent project for both and one can run all the integration
tests by using this parent project.

(cherry picked from commit c09f4a0)
matriv added a commit that referenced this issue May 22, 2020
Move the JDBC functionality integration tests from `:sql:qa` to a separate
module `:sql:qa:jdbc`. This way the tests are isolated from the rest of the
integration tests and they only depend to the `:sql:jdbc` module, thus
removing the danger of accidentally pulling in some dependency that may
hide bugs.

Moreover this is a preparation for #56722, so that we can run those tests
between different JDBC and ES node versions and ensure forward
compatibility.

Move the rest of existing tests inside a new `:sql:qa:server` project, so that
the `:sql:qa` becomes the parent project for both and one can run all the integration
tests by using this parent project.

(cherry picked from commit c09f4a0)
matriv added a commit that referenced this issue May 22, 2020
Move the JDBC functionality integration tests from `:sql:qa` to a separate
module `:sql:qa:jdbc`. This way the tests are isolated from the rest of the
integration tests and they only depend to the `:sql:jdbc` module, thus
removing the danger of accidentally pulling in some dependency that may
hide bugs.

Moreover this is a preparation for #56722, so that we can run those tests
between different JDBC and ES node versions and ensure forward
compatibility.

Move the rest of existing tests inside a new `:sql:qa:server` project, so that
the `:sql:qa` becomes the parent project for both and one can run all the integration
tests by using this parent project.

(cherry picked from commit c09f4a0)
@matriv
Copy link
Contributor

matriv commented Jun 8, 2020

@mark-vieira @rjernst Can we please have an ETA for this?

The JDBC tests are already split into a separate jdbc module under sql/qa and we really need this soon as there is already a feature (date_nanos) blocked by it and soon we want to add array support which also needs JDBC changes.

Thank you.

@mark-vieira
Copy link
Contributor Author

I don't have a concrete ETA for you but I've thrown this on the top of the list for upcoming work.

@mark-vieira
Copy link
Contributor Author

@matriv I presume that the set of tests that we want to perform compatibility testing with are everything under :x-pack:plugin:sq:qa:jdbc. Essentially the suite of tests represented by this build: https://gradle-enterprise.elastic.co/s/5sd7brkq23oi6/tests?collapse-all

The tests in :x-pack:plugin:sql:jdbc are simply unit tests, so compatibility testing is not applicable there. Correct?

@matriv
Copy link
Contributor

matriv commented Jun 18, 2020

@mark-vieira Correct! only the :x-pack:plugin:sq:qa:jdbc

@mark-vieira
Copy link
Contributor Author

mark-vieira commented Jun 25, 2020

One thing I ran into here is that it's going to be problematic to use the shadowjar as the test artifact here. To do so, we have to exclude any dependencies from the test classpath that are bundled in the JDBC driver fatjar (things like core libs, etc). The problem is, then we use the libraries from the BWC jar when testing, which causes errors like this:

https://gradle-enterprise.elastic.co/s/cxm6mvbcixo4c/tests/:x-pack:plugin:sql:qa:jdbc:single-node:v7.8.1%23bwcTest/org.elasticsearch.xpack.sql.qa.jdbc.single_node.JdbcFetchSizeIT/initializationError#1

We are relying on the JDBC driver jar to bring in that class, but it doesn't exist in 7.8 and thus the test fails. I'm not sure we have a good solution here other than to use the "slim" jar for testing and otherwise use the ES dependencies from the current branch. This is bound to cause other issues though, as we are likely to have incompatibilities between the two.

@mark-vieira
Copy link
Contributor Author

mark-vieira commented Jun 25, 2020

I'm struggling to find a good way forward here given the way these tests are currently architected. The problem is our :test:framework code brings in :server and other core libraries which are going to clash with the same classes coming in from the JDBC driver jar. In my mind we have two options here if we want to do compatibility testing with older JDBC driver jars:

  1. We rearchitect the tests such that they load the JDBC driver fatjar in some kind of isolated classloader that can't see the rest of the test runner classpath.
  2. We relocate org.elasticsearch classes in the JDBC driver jar under a different package so they can both exist on the same classpath simultaneously.

@mark-vieira
Copy link
Contributor Author

I'll add a third item:

  1. We don't use ESRestTestCase, remove the dependency on :test:framework altogether and directly extend LuceneTestCase for these tests. I don't see much need to use ESRestTestCase anyhow.

@mark-vieira
Copy link
Contributor Author

Ok item (3) likely won't work since these tests rely on RestClient to setup test data.

@costin
Copy link
Member

costin commented Jun 28, 2020

Thanks for looking into this @mark-vieira. Both (1) and (2) work for me - I have a slight preference towards (2) since, despite not being a fan of shadowing packages, medium term this might prove beneficial as embedding JDBC drivers in other Java apps that use ES, will lead to the same problem that you are facing - classpath issues.
So moving all packages out of the way would address both problems.

The concern that I have though is this feature will be available only moving forward so past releases (in this case 7.7) would miss out on this. However I'd rather have us acting quickly through (2) and then look into ways to address 7.7 later down the road if we really have to.

So 👍 for going with (2).

@mark-vieira
Copy link
Contributor Author

The concern that I have though is this feature will be available only moving forward so past releases (in this case 7.7) would miss out on this.

Correct, with this option we effectively cannot do compatibility testing with existing releases. We would have to start with 7.8.1 given that would be the first available version with relocated packages. That means that we will not know if we break backcompat with the 7.7 driver (although 7.8.1 will be a pretty close analog).

@matriv
Copy link
Contributor

matriv commented Jun 30, 2020

Even though this is not ideal, we can assume that is safe enough to start with 7.8.1, so if (1) is difficult to implement, it's fine also by me to go with the (2) approach.

@mark-vieira
Copy link
Contributor Author

so if (1) is difficult to implement, it's fine also by me to go with the (2) approach

To clarify, I've not investigated what it would take to implement solution (1). I suspect someone more familiar with the tests would be better suited to do that. It might be worth looking into in parallel with this. Course (2) is not necessarily an easy win, package relocation might not easily work, as it doesn't play nicely with things like reflection. We may find out that we have to make code changes in the JDBC driver or some of it's dependencies to make that work, at which point refactoring the tests might be an easier road.

@costin
Copy link
Member

costin commented Jul 1, 2020

Course (2) is not necessarily an easy win, package relocation might not easily work, as it doesn't play nicely with things like reflection. We may find out that we have to make code changes in the JDBC driver or some of it's dependencies to make that work, at which point refactoring the tests might be an easier road.

Have you tried changing the packages and encountered failures? The JDBC driver is not using reflection nor does it depend on the package name; maybe the org.elasticsearch dependency does but this would be easy to find out by just trying things out.

@mark-vieira
Copy link
Contributor Author

Have you tried changing the packages and encountered failures?

I have not yet, I'll be giving this a try soon.

@mark-vieira
Copy link
Contributor Author

Ok, with some tinkering I was able to get a green build of the current integration test suite against a shadowjar with relocated org.elasticsearch packages.

https://gradle-enterprise.elastic.co/s/3cutzwlvii6lq/tests

I'll move on to testing this against similarly configured BWC branch as well.

@mark-vieira
Copy link
Contributor Author

mark-vieira commented Jul 8, 2020

Here we are folks. This is a successful run of building the 7.8.1 JDBC driver (with relocated shadowed dependencies) and testing against a 7.9 Elasticsearch cluster.

https://gradle-enterprise.elastic.co/s/sqjzlft75tx42/tests

Since this approach seems to be ok I'll move forward with getting the necessary changes made in the 7.8 branch and forward. I'd also highly suggest we expand the existing test suite to also validate the current JDBC driver version is what we expect. I'm going to inject a jdbc.driver.version system property into the test runner with the "expected" JDBC jar version. We should add a unit test to validate that matches the reality of what's on the test classpath.

@mark-vieira
Copy link
Contributor Author

Alright with #60430 merged we now have this in place. We didn't make this in to 7.8 so we are beginning compatibility testing with 7.9. As mentioned above, I would highly recommend putting some assertions in place to confirm the JDBC driver being loaded is the version that's expected in case we muck up the build wiring somehow.

@mark-vieira mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/SQL SQL querying :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Development

No branches or pull requests

5 participants