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

PARQUET-2371: Resolve japicmp CI failure #1181

Merged
merged 2 commits into from
Nov 3, 2023

Conversation

amousavigourabi
Copy link
Contributor

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    No user facing changes, only resolves CI failure.

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

Japicmp failed on newly introduced classes, during review this did not cause a failure. This likely has something to do with the japicmp 0.18.1 update we pushed since then (for this, see #1176). Includes bumping japicmp to 0.18.2.
The false positives were all under the banner of CLASS_GENERIC_TEMPLATE_CHANGED. Similar problems seem to have been reported before at siom79/japicmp#345.

@amousavigourabi
Copy link
Contributor Author

@ConeyLiu @wgtmac, this is a patch for the problems we discussed at #1141.

@@ -68,7 +68,7 @@
<jackson.package>com.fasterxml.jackson</jackson.package>
<jackson.version>2.15.2</jackson.version>
<jackson-databind.version>2.15.2</jackson-databind.version>
<japicmp.version>0.18.1</japicmp.version>
<japicmp.version>0.18.2</japicmp.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

After reverting the version back to 0.16.0, the problem is gone.

Copy link
Contributor

@ConeyLiu ConeyLiu left a comment

Choose a reason for hiding this comment

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

I am fine with these changes if we want to keep the japicmp version. Or else we should revert the version change.

Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

+1 on my side.

@Fokko @gszadovszky Do you have any comment?

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@wgtmac
Copy link
Member

wgtmac commented Nov 3, 2023

I'll merge this when the CI is green to unblock CIs of other PRs.

@wgtmac wgtmac merged commit 7c6758b into apache:master Nov 3, 2023
9 checks passed
@gszadovszky
Copy link
Contributor

Sorry @wgtmac, @ConeyLiu, for the late response.
I don't think it is a bug in japicmp to fail with a new method in an existing interface. It would fail a 3rd party if it implements the interface. To avoid keep excluding files we might add a default implementation for the new methods instead, if possible.

@amousavigourabi
Copy link
Contributor Author

@gszadovszky, the issue at hand was not that adding methods to already existing interfaces caused failures. As you noted, this would be perfectly fine and expected behaviour. Instead, japicmp failed on wholly new interfaces (see org.apache.parquet.conf.ParquetConfiguration), and new method implementations (see org.apache.parquet.hadoop.thrift.TBaseWriteSupport), under quite a vague motivation CLASS_GENERIC_TEMPLATE_CHANGED.

@gszadovszky
Copy link
Contributor

Thanks for the clarification, @amousavigourabi. We cannot do anything but excluding these classes, then, I'm afraid. But the comment is a bit misleading.
Have you tried creating a bug for japicmp about it?

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

Successfully merging this pull request may close these issues.

4 participants