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 #17691

Merged
merged 15 commits into from
Sep 28, 2022

Conversation

youzipi
Copy link
Contributor

@youzipi youzipi commented Sep 16, 2022

Master Issue: #12271 #16991

Motivation

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

Modifications

  • fix violations in pulsar-broker package.

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

Matching PR in forked repository

PR in forked repository: youzipi#1

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

youzipi commented Sep 23, 2022

@MarvinCai @tisonkun

import com.google.common.collect.Range;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.function.Predicate;
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. As we enable Modernizer for managed-ledger already, doesn't this file cause failure already?

Copy link
Contributor Author

@youzipi youzipi Sep 26, 2022

Choose a reason for hiding this comment

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

image

 /Users/mac/projects/source_code/pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription$3.java:-1: Prefer java.util.function.Predicate

here is the only one guava Predicate in PersistentSubscription, an anonymous class:

return cursor.scan(position, new Predicate<Entry>() {
@Override
public boolean apply(Entry entry) {
if (log.isDebugEnabled()) {
log.debug("found {}", entry);
}


moderinzer use ASM to scan the class files and detect the latency usage.

current rule only detect implementing legacy interfaces, see gaul/modernizer-maven-plugin#1

I build a test case and debug it,
found that modernizer use a map to store all rules, and match with the class token by absolutely equal(not contains):

private final Map<String, Violation> violations;
...
Violation violation = violations.get(token); 
checkToken(token, violation, name, lineNumber);

actually, this case is detected in the interface list,
that is where the lineNumber=-1 from:

        for (String itr : interfaces) {
            Violation violation = violations.get(itr);
            checkToken(itr, violation, name, /*lineNumber=*/ -1);
        }

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. One comment out of curiosity.

@youzipi
Copy link
Contributor Author

youzipi commented Sep 27, 2022

@MarvinCai @codelipenghui

@MarvinCai
Copy link
Contributor

/pulsarbot run-failure-checks

@codelipenghui codelipenghui added this to the 2.12.0 milestone Sep 28, 2022
@lhotari lhotari merged commit 8aef1bf into apache:master Sep 28, 2022
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 ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants