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

[cleanup][broker][Modernizer] fix violations in pulsar-broker #17275

Merged
merged 9 commits into from
Sep 15, 2022

Conversation

youzipi
Copy link
Contributor

@youzipi youzipi commented Aug 25, 2022

Master Issue: #12271 #16991

Motivation

Apply Maven Modernizer plugin to enforce we move away from legacy APIs.

Modifications

fix violations in pulsar-broker

Verifying this change

This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • doc-not-needed

@youzipi
Copy link
Contributor Author

youzipi commented Aug 29, 2022

/pulsarbot run-failure-checks

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 29, 2022
@youzipi
Copy link
Contributor Author

youzipi commented Aug 30, 2022

/pulsarbot run-failure-checks

@@ -71,7 +71,7 @@ public void testFilterOutForPreInterceptFilter() throws Exception {
Mockito.doNothing().when(chain).doFilter(Mockito.any(), Mockito.any());
HttpServletRequestWrapper mockInputStream = new MockRequestWrapper(request);
Mockito.doReturn(mockInputStream.getInputStream()).when(request).getInputStream();
Mockito.doReturn(new StringBuffer("http://127.0.0.1:8080")).when(request).getRequestURL();
Mockito.doReturn(new StringBuilder("http://127.0.0.1:8080")).when(request).getRequestURL();
Copy link
Contributor

Choose a reason for hiding this comment

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

method signature is StringBuffer getRequestURL();, so can't use StringBuilder 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.

yes, I saw the failed case.
I plan to exclude this rule.

I created an issue:
gaul/modernizer-maven-plugin#149
they insist this as a default rule.

Copy link
Contributor

@MarvinCai MarvinCai left a comment

Choose a reason for hiding this comment

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

lgtm

@youzipi
Copy link
Contributor Author

youzipi commented Sep 8, 2022

/pulsarbot run-failure-checks

1 similar comment
@youzipi
Copy link
Contributor Author

youzipi commented Sep 8, 2022

/pulsarbot run-failure-checks

@youzipi
Copy link
Contributor Author

youzipi commented Sep 9, 2022

I can't trigger the checks.
because it was cancelled manually?
@MarvinCai @tisonkun

@tisonkun
Copy link
Member

tisonkun commented Sep 9, 2022

I think there were some CI changes that happened yesterday, you can merge the latest master and things should be fine. Ping me if it still doesn't work for you.

@youzipi
Copy link
Contributor Author

youzipi commented Sep 9, 2022

I think there were some CI changes that happened yesterday, you can merge the latest master and things should be fine. Ping me if it still doesn't work for you.

@tisonkun
I merge the master today, but is shows only 5 checks:
image

@tisonkun
Copy link
Member

tisonkun commented Sep 9, 2022

@youzipi this is correct. Other tasks will be scheduled after the prerequisites are finished. And now we squash all required tasks into two final steps (Pulsar CI checks completed & cpp-tests).

@youzipi
Copy link
Contributor Author

youzipi commented Sep 13, 2022

@codelipenghui

@youzipi
Copy link
Contributor Author

youzipi commented Sep 15, 2022

@tisonkun

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. ping @codelipenghui

@codelipenghui codelipenghui merged commit ff49c5c into apache:master Sep 15, 2022
<ignorePackage>org.apache.pulsar.client</ignorePackage>
</ignorePackages>
<exclusionPatterns>
<exclusionPattern>java/lang/StringBuffer."&lt;init&gt;":.*</exclusionPattern>
Copy link
Member

Choose a reason for hiding this comment

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

@youzipi may I ask the reason you add this pattern?

Copy link
Contributor Author

@youzipi youzipi Sep 15, 2022

Choose a reason for hiding this comment

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

@tisonkun
there is a conflict between mockito and moderinizer:
moderinizer prefers StringBuilder instead of StringBuffer always,
but Mockito.doReturn(new StringBuilder("http://127.0.0.1:8080")).when(request).getRequestURL(); will fail in CI as type not match.

former discussion:
#17275 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants