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

Add compatibility testing for JDBC driver #59322

Closed

Conversation

mark-vieira
Copy link
Contributor

This commit adds compatibility testing of our JDBC driver against
different Elasticsearch versions. Although we are really testing the
forwards compatibility nature of the JDBC driver we model the testing
the same as we do existing BWC tests, that is, with the current branch
fetching the earlier versions of the artifact that is to be tested. In
this case, that's the JDBC driver itself.

Because the tests include the JDBC driver jar on it's classpath we had
to change the packaging of the driver jar in order to avoid jarhell and
other conflicting dependency issues when using an old JDBC driver with
later branches. For this we simply relocate all driver dependencies in
the shadow jar under a "shadowed" package. This allows the JDBC driver
to use the correct version of Elasticsearch libs classes, while the
tests themselves use their versions. Since this required a change to the
driver jar compatibility testing can only go back as far as that version
which at the time of this commit is 7.8.1.

We are starting with merging this into the 7.8 branch since due to the
"forwards" nature of this change we have to do it in reverse order we'd
normally merge PRs in. So with the changes to the JDBC jar packaging
in 7.8, we'll be able to "forwardport" this to 7.x and then finally to master.

Closes #56722

This commit adds compatibility testing of our JDBC driver against
different Elasticsearch versions. Although we are really testing the
forwards compatibility nature of the JDBC driver we model the testing
the same as we do existing BWC tests, that is, with the current branch
fetching the earlier versions of the artifact that is to be tested. In
this case, that's the JDBC driver itself.

Because the tests include the JDBC driver jar on it's classpath we had
to change the packaging of the driver jar in order to avoid jarhell and
other conflicting dependency issues when using an old JDBC driver with
later branches. For this we simply relocate all driver dependencies in
the shadow jar under a "shadowed" package. This allows the JDBC driver
to use the correct version of Elasticsearch libs classes, while the
tests themselves use their versions. Since this required a change to the
driver jar compatibility testing can only go back as far as that version
which at the time of this commit is 7.8.1.
@mark-vieira mark-vieira added :Delivery/Build Build or test infrastructure :Analytics/SQL SQL querying labels Jul 9, 2020
@mark-vieira mark-vieira requested a review from matriv July 9, 2020 17:26
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jul 9, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 9, 2020
@mark-vieira mark-vieira requested a review from rjernst July 9, 2020 17:26
@mark-vieira
Copy link
Contributor Author

mark-vieira commented Jul 9, 2020

Note, that because this branch represents 7.8.1 and this is also the first versions to support compatibility testing, there won't actually be any BWC tests executed for JDBC here. Once this is merged back into 7.x we'll start seeing compatibility tests run. However, the infrastructure is still needed here as tests will start running once we release 7.8.1 and this branch becomes 7.8.2.

Confused yet? 😉

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @mark-vieira for this effort!

apply plugin: 'elasticsearch.testclusters'
apply plugin: 'elasticsearch.rest-test'
// Configure compatibility testing tasks
for (Version bwcVersion : BuildParams.bwcVersions.jdbcDriverCompatible) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd to me that the list of versions compatible here would live in BwcVersions. Could we instead do the filtering here within the jdbc test? I think that class should stay generic, not containing references to any specific projects. One reason for that is if I'm trying to look at what versions we are testing jdbc, I'm likely to look in this file, so I expect to see any deviation from all versions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention was that this class contains the logic for determining types of compatibility. So index, wire, etc. The thought is that JDBC driver compatibility is just another form of that, so why deviate from the pattern and bury that in the JDBC QA project? The class is still generic by that definition since doesn't actually reference any Gradle projects.

Copy link
Member

@rjernst rjernst Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except in this case, since we have just recently started compatibility support, when would it deviate from index compatible versions (also, is index compatible correct?)? When we branch 8.x, wouldn't this in master be compatible with all index compatible versions? I don't see this as another type of compatibility, I see it as another feature of the system that should be aligned with existing compatibility rules.

Copy link
Contributor Author

@mark-vieira mark-vieira Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment on whether or not JDBC driver compatibility should strictly align with index compatibility or not, you'd have to reach out to the SQL team for that. But in my mind, even if it is essentially the same, it didn't feel right to highjack that definition as I'm not a big fan of overloading terms. The introduction of JDBC compatibility is new, and the semantics might change, so I thought it best to be explicit. @matriv perhaps you could clarify exactly what the JDBC driver compatibility constraints will be going forward?

Also, FWIW, there are other definitions of compatibility that deviate from index/wire compatibility. We are having this discussion right now regarding things like cross cluster search. I suspect once we have a concrete definition and implement proper testing that we'd similarly define the set of compatible versions in BwcVersions as that's what I interpret it's purpose is for. That is, to answer the question "what versions of Elasticsearch should scenario/feature X work with?".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to that, REST compatibility testing falls in the same bucket. The versions tested will likely be identical to index compatibility, but it seems more correct to again model it as a separate thing, at least until we one day arrive at some kind of universal "client compatible" definition, which seems a ways off.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mark-vieira I agree it is the job of BwcVersions to describe compatibility. But as you say, major-1 is the same as index compatibility. Ignoring the names, we effectively have 2 compatibility sets in ES, both described in code: index compatibility (major - 1), and wire compatibility (major-1.last). My push back here is to creating extra concepts that don't need to exist. I think the compatibility of any part of the system should match with one of those two concepts. Thus far we've called it index compatibility in this case. Giving jdbc it's own version set here makes it confusing both because it would not exist in code outside gradle (see Version.java in elasticsearch.jar) and diverging from one of those two concepts would add cognitive load to users and developers in understanding compatibility. Thus, I still think we should avoid creating a new set here which (after the initial version) should not differ from one of those two sets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm perfectly content with that if we agree that client compatibility = index compatibility and that we agree to maintain that going forward. So far we've been discussion client compatibility as it's own thing, and only incidentally does it overlap with index compatibility. That is, the decision to make ensure client compatibility across the versions we do isn't so much to align with index compatibility, but really to support upgrades in a similar way to index compatibility. By that definition "index compatibility" is really a poor name but we don't have to address that here.

@bpintea Does what I saw above make sense? That is, the use cases we wish to address with "major - 1" compatibility with clients is the same as the reason we ensure index compatibility across the same versions. That is, to make sure that folks can go from any minor version of a given major, to any minor version of the next major without having to do any intermediary upgrades (as is the case for zero-downtime rolling upgrades).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that since currently the scheme we support regarding JDBC fwds compatibility matches the one about index compatibility we can proceed with that and if something changes re-open this discussion in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpintea Does what I saw above make sense? That is, the use cases we wish to address with "major - 1" compatibility with clients is the same as the reason we ensure index compatibility across the same versions.

Sure, it does.
And while not sure if they don't actually stem from the same place, I'm actually glad that the index bwc policy overlaps with clients', hoping for easier maintenance and better testing coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I'll push forward with that then. That said, I think we've missed the boat for getting this in 7.8. Sorry for the delay, I just think it makes sense for us all to be on the same page here. Although we've already branched 7.9 I'm going to get this merged in there, as the JDBC jar changes are necessary in order to have testing in 7.x. So I'll close this PR for now and open a new one against 7.9.

@rjernst Given the clarifications from @matriv and @bpintea I'm good with piggybacking on index compatibility as the definition for this as well. I'll modify the 7.9 PR appropriately for that.

@mark-vieira mark-vieira deleted the 7.8-jdbc-compatibility-testing branch July 22, 2020 18:27
@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

Successfully merging this pull request may close these issues.

5 participants