[CALCITE-5136] Avatica build (or CI) must fail if there are deprecation warnings #272

chrisdennis · 2025-01-28T17:33:33Z

Fixes applied are:

move new URL(...) to new URI(...).toUrl()
move Assert.assertThat(...) to MatcherAssert.assertThat(...)
move ExpectedException ~~to Assert.assertThrows(...)~~ use manual code since Guava version changes cause the code to see both 4.12 and 4.13.1 JUnit versions.
in all other cases either propagate the deprecation or suppress it.

stoty · 2025-02-11T16:18:16Z

move ExpectedException to Assert.assertThrows(...) use manual code since Guava version changes cause the code to see both 4.12 and 4.13.1 JUnit versions.

Are you sure ? We're dependency managing Junit to 4.12 , and I could not find 4.13.1 in the gradle dependecy output anywhere ?

We should upgrade to 4.13.2 anyway (in a different JIRA)

chrisdennis · 2025-02-11T17:23:02Z

Are you sure ? We're dependency managing Junit to 4.12 , and I could not find 4.13.1 in the gradle dependecy output anywhere ?

I could have sworn this failed locally for me... but now I can't reproduce that failure. I've pulled that change and pushed. Let's see what CI makes of it now.

stoty · 2025-02-11T18:51:08Z

server/src/test/java/org/apache/calcite/avatica/server/HttpServerSpnegoWithoutJaasTest.java

  private static boolean isKdcStarted = false;
  private static boolean isHttpServerStarted = false;

-  private static URL httpServerUrl;


I think we should update the names in these cases.

Pushed the rename. I think this was the only case where we changed the type.

chrisdennis · 2025-02-11T19:01:35Z

FYI: I pounded on the test that failed on macOS locally and see the following pattern:

Failure Statistics
49976 : connection refused
24 : connection reset

The existing code doesn't account for "connection reset". I have a fix for this... does it need a JIRA issue, and a separate PR?

stoty · 2025-02-11T19:52:41Z

Thank you.

Preferably yes. CALCITE-6799 already exists for the issue.

stoty · 2025-02-11T19:55:08Z

core/src/main/java/org/apache/calcite/avatica/util/Sources.java

          filePath += "/";
        }
        try {
          // We need to encode path. For instance, " " should become "%20"


nit:
Is this comment still needed ?

No, probably not. Removed... but closer inspection here forced me to fix something else. This branch of the code is making a very "odd" URI/URL. It's a hierarchical URI, with a relative path and a scheme. I don't know if they're illegal under the RFC... but it's certainly weird. I had to subtly tweak things here to construct one by treating it as a non-heirarchical URL (args here are: scheme, scheme-specific-part, fragment). The behavior now matches the old code.

stoty

+1 LGTM

rubenada · 2025-02-13T08:47:14Z

This PR looks in a good shape, shall we move on?
One minor detail though, per our contribution guidelines, commit messages should not end with a period; apart from that I think generally we tend to use as final commit message the Jira title, i.e. [CALCITE-5136] Avatica build (or CI) must fail if there are deprecation warnings in this case.

chrisdennis · 2025-02-13T13:38:36Z

One minor detail though, per our contribution guidelines, commit messages should not end with a period; apart from that I think generally we tend to use as final commit message the Jira title, i.e. [CALCITE-5136] Avatica build (or CI) must fail if there are deprecation warnings in this case.

Fixed.

chrisdennis · 2025-02-13T13:47:28Z

@stoty something is up in CI... the force push didn't change anything: https://github.com/apache/calcite-avatica/compare/da93125c4a177717611beb1db0b51f3bf279e390..fd947c724f8a46ec45f5126adcefdeb4868e25eb

~~Yet somehow everything is now failing... and it looks like it's because the build resolved 4.13.x of JUnit (evidenced in all the assertThat(...) deprecation warnings).~~

Should have rebased... missed that you updated to 4.13.2. Will adjust accordingly

stoty · 2025-02-18T08:48:34Z

.idea/vcs.xml

-        <mapping directory="$PROJECT_DIR$" vcs="Git" />
-    </component>
-</project>
+  <component name="CommitMessageInspectionProfile">


Did you mean to include this @chrisdennis ?

It doesn't seem related to the deprecations.

I did not... I guess IntelliJ got over-eager with the reformatting and I did an ill-advised git add ..

…on warnings Fixes applied are: * move `new URL(...)` to `new URI(...).toUrl()` * move `Assert.assertThat(...)` to `MatcherAssert.assertThat(...)` * move `ExpectedException` to `Assert.assertThrows(...)` * in all other cases either propagate the deprecation or suppress it.

stoty · 2025-02-18T14:36:28Z

merged manually.

chrisdennis force-pushed the calcite-5136 branch from b6f86d9 to 8b1efc6 Compare January 28, 2025 18:53

chrisdennis force-pushed the calcite-5136 branch 2 times, most recently from 0435ac9 to 9ac4eb7 Compare February 11, 2025 14:48

chrisdennis force-pushed the calcite-5136 branch from 9ac4eb7 to 3a02a90 Compare February 11, 2025 17:22

stoty reviewed Feb 11, 2025

View reviewed changes

chrisdennis force-pushed the calcite-5136 branch from 3a02a90 to d51c487 Compare February 11, 2025 18:58

stoty reviewed Feb 11, 2025

View reviewed changes

stoty approved these changes Feb 11, 2025

View reviewed changes

chrisdennis force-pushed the calcite-5136 branch from d51c487 to da93125 Compare February 11, 2025 20:55

chrisdennis force-pushed the calcite-5136 branch from da93125 to fd947c7 Compare February 13, 2025 13:36

chrisdennis changed the title ~~[CALCITE-5136] Elevate deprecation warnings to error and handle resultant failures.~~ [CALCITE-5136] Avatica build (or CI) must fail if there are deprecation warnings Feb 13, 2025

chrisdennis force-pushed the calcite-5136 branch from fd947c7 to 28c98b0 Compare February 13, 2025 14:19

F21 force-pushed the main branch 7 times, most recently from 8f21bf5 to 4d3aaac Compare February 17, 2025 01:08

stoty reviewed Feb 18, 2025

View reviewed changes

chrisdennis force-pushed the calcite-5136 branch from 28c98b0 to b5bc68e Compare February 18, 2025 14:06

stoty closed this Feb 18, 2025

[CALCITE-5136] Avatica build (or CI) must fail if there are deprecation warnings #272

[CALCITE-5136] Avatica build (or CI) must fail if there are deprecation warnings #272

Conversation

chrisdennis commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty commented Feb 11, 2025

Uh oh!

chrisdennis commented Feb 11, 2025

Uh oh!

stoty Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

chrisdennis Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

chrisdennis commented Feb 11, 2025

Uh oh!

stoty commented Feb 11, 2025

Uh oh!

stoty Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

chrisdennis Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

rubenada commented Feb 13, 2025

Uh oh!

chrisdennis commented Feb 13, 2025

Uh oh!

chrisdennis commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisdennis Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

chrisdennis Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

stoty commented Feb 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisdennis commented Jan 28, 2025 •

edited

Loading

chrisdennis Feb 11, 2025 •

edited

Loading

chrisdennis commented Feb 13, 2025 •

edited

Loading

stoty Feb 18, 2025 •

edited

Loading