Skip to content

Comments

Bump avatica-core from 1.17.0 to 1.22.0#12872

Closed
dependabot[bot] wants to merge 3 commits intomasterfrom
dependabot/maven/org.apache.calcite.avatica-avatica-core-1.22.0
Closed

Bump avatica-core from 1.17.0 to 1.22.0#12872
dependabot[bot] wants to merge 3 commits intomasterfrom
dependabot/maven/org.apache.calcite.avatica-avatica-core-1.22.0

Conversation

@dependabot
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Aug 6, 2022

Bumps avatica-core from 1.17.0 to 1.22.0.

Commits
  • 71fc0ab [CALCITE-5220] Release Avatica 1.22
  • 0c097b6 [CALCITE-5218] Verify HTTP client class before instantiating it
  • aad227f Checkout release svn repository when promoting a release using the docker script
  • bad14c7 Update website for Avatica 1.21.0 release
  • 0883262 [CALCITE-5097] Release Avatica 1.21.0
  • 360c0e7 [CALCITE-5095] Support Java 18 and Guava 31.1-jre
  • 05658fe [CALCITE-5116] Upgrade vlsi-release-plugins to 1.78
  • 1f0f0c1 [CALCITE-4147] Rename "master" branch to "main"
  • 7f9844c [CALCITE-5108] Make website GDPR-compliant
  • 7e168f2 [CALCITE-5106] Upgrade to Jekyll 4 and remove unnecessary dependencies from g...
  • Additional commits viewable in compare view

Dependabot compatibility score

You can trigger a rebase of this PR by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
    You can disable automated security fix PRs for this repo from the Security Alerts page.
> **Note** > Automatic rebases have been disabled on this pull request as it has been open for over 30 days.

Bumps [avatica-core](https://github.com/apache/calcite-avatica) from 1.17.0 to 1.22.0.
- [Release notes](https://github.com/apache/calcite-avatica/releases)
- [Commits](apache/calcite-avatica@rel/avatica-1.17.0...rel/avatica-1.22.0)

---
updated-dependencies:
- dependency-name: org.apache.calcite.avatica:avatica-core
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot dependabot bot added Area - Dependencies java Pull requests that update Java code labels Aug 6, 2022
@FrankChen021
Copy link
Member

The upgrade is disabled, see #13160

@FrankChen021
Copy link
Member

Oh, I mistook avatica as calcite, ignored the comment above.

@zachjsh
Copy link
Contributor

zachjsh commented Oct 6, 2022

Looks like it is failing code coverage, weird, no code was added / removed. Tried restarting those jobs, lets see.

@zachjsh
Copy link
Contributor

zachjsh commented Oct 6, 2022

@dependabot rebase

@dependabot @github
Copy link
Contributor Author

dependabot bot commented on behalf of github Oct 6, 2022

Looks like this PR has been edited by someone other than Dependabot. That means Dependabot can't rebase it - sorry!

If you're happy for Dependabot to recreate it from scratch, overwriting any edits, you can request @dependabot recreate.

…g.apache.calcite.avatica-avatica-core-1.22.0
@clintropolis
Copy link
Member

Looks like it is failing code coverage, weird, no code was added / removed. Tried restarting those jobs, lets see.

Ah, the coverage always runs, even when the tests fail, it looks like some of the tests are failing: https://app.travis-ci.com/github/apache/druid/jobs/584853001#L1978

@paul-rogers
Copy link
Contributor

Looked into the issues. There are several. Upshot: we cannot upgrade to this version: Avatica itself needs some fixes. We may have compatibility issues when older Avatica clients talk with newer Avatica servers.

Version 1.22 appears to be a bit better at enforcing types. This has introduce multiple subtle failures in the tests, in part because Druid seems to be working around prior issues and those workarounds now seem to be causing problems. All of the issues are related to type propagation.

Let's start with Protobuf. This protocol preserves the Java types we define in the server. Some tests fail because, before 1.22, the data sent across the wire would change type: Double would get deserialized as Float, say. With 1.22, tests have to be updated to expect Double values if we say that the column is of type DOUBLE. However, this uncovers test flaws. We expect fractional floating point values to compare exactly. They were equal when the type was converted to Float, but are no longer equal when the full Double type is preserved.

Druid supports both the Prototbuf and JSON protocols. While Protobuf appears to preserve types, JSON does not: we're at the mercy of whatever Jackson chooses as the deserialized type. This is related to the fact that JSON has only a single number type: there is some fudging required to map to a Java type. The result is that we tell Avatica that the type is BIGINT (Long), but Jackson deserializes the value as Integer if it is small. This seems to work OK for scalar values, but in 1.22, it causes problems with arrays.

Avatica expects that the elements in an array match the defined type. But, with JSON, we create an array of Long on the server, but Jackson deserializes them to an array of Integer on the client. Avatica attempts to cast the item to a Long, which fails. It would seem that Avatica should expect mixed Integer and Long values in this case, casting them to Long

Druid appearently tried to work around this by declaring the Java type of some numbers to be Number. While this worked in 1.17, it does not work in 1.22. We have to declare the specific type.

Thus, when doing this upgrade, we have four issues:

  • Avatica is broken. We probably have to submit a patch to get arrays to work with JSON. We can then consider using 1.23 (or later) with the patch.
  • Druid has to change to match the stricter type rules for 1.22.
  • Tests have to change to match the new behavior, and cannot expect that Double values compare exactly.
  • Compatibility

With the last item, a worry is compatibility. Some of the changes do suggest that newer servers may not work with older clients. We cannot expect users to upgrade their clients concurrently with Druid: some cross-version interoperation is necessary. Thus, we need testing, which is a bit hard to do as a unit test or integration test.

Net review is -1 on this change until we resolve the above issues.

@dependabot @github
Copy link
Contributor Author

dependabot bot commented on behalf of github Jan 20, 2023

A newer version of these dependencies exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged.

@xvrl
Copy link
Member

xvrl commented Aug 15, 2023

looks like this has been closed as part of #14510, can you confirm @somu-imply ?

@xvrl xvrl mentioned this pull request Aug 15, 2023
10 tasks
@somu-imply
Copy link
Contributor

When we upgraded to 1.23 we did not see any of the issues we saw in the Travis failure here. To reconfirm we are triggering a test with both the old and new versions of Avatica. I'll close this PR when we have the results. But so far everything looks good and the CI on #14510 did not see any issues

@somu-imply
Copy link
Contributor

We finished our runs, and things look good. Here is what we know about Druid upgrading to Avatica 1.23.0 :
The customer can continue to use older versions of Calcite client jar file. We tested 1.19.0 and that works fine. They do not need to do anything if they are using a version that is 1.20.0 or lower. If the customer uses a version that is 1.21.0 or higher, they will have to add the connection option transparent_reconnection=true. See https://calcite.apache.org/avatica/docs/client_reference.html#transparent_reconnection. This issue can be closed

@xvrl
Copy link
Member

xvrl commented Aug 22, 2023

Thanks @somu-imply I will close the issue here. Do you mind adding a comment to the PR in #14510 to ensure we mention the compatibility requirements as part of the release notes? Thanks!

@xvrl xvrl closed this Aug 22, 2023
@dependabot @github
Copy link
Contributor Author

dependabot bot commented on behalf of github Aug 22, 2023

OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting @dependabot ignore this major version or @dependabot ignore this minor version. You can also ignore all major, minor, or patch releases for a dependency by adding an ignore condition with the desired update_types to your config file.

If you change your mind, just re-open this PR and I'll resolve any conflicts on it.

@dependabot dependabot bot deleted the dependabot/maven/org.apache.calcite.avatica-avatica-core-1.22.0 branch August 22, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Dependencies java Pull requests that update Java code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants