You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/16 08:40:56 UTC
[GitHub] [pulsar] youzipi opened a new pull request, #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
youzipi opened a new pull request, #17691:
URL: https://github.com/apache/pulsar/pull/17691
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
- [x] `doc-not-needed`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] youzipi commented on a diff in pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on code in PR #17691:
URL: https://github.com/apache/pulsar/pull/17691#discussion_r979728197
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java:
##########
@@ -18,13 +18,13 @@
*/
package org.apache.bookkeeper.mledger;
-import com.google.common.base.Predicate;
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;
Review Comment:
```
/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:
https://github.com/apache/pulsar/blob/e73b29863614a210d14e105c26f691c29d0548d5/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L558-L563
---
moderinzer use ASM to scan the class files and detect the latency usage.
current rule only detect `implementing legacy interfaces`, see https://github.com/gaul/modernizer-maven-plugin/issues/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`):
```java
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:
```java
for (String itr : interfaces) {
Violation violation = violations.get(itr);
checkToken(itr, violation, name, /*lineNumber=*/ -1);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] youzipi commented on a diff in pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on code in PR #17691:
URL: https://github.com/apache/pulsar/pull/17691#discussion_r979728197
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java:
##########
@@ -18,13 +18,13 @@
*/
package org.apache.bookkeeper.mledger;
-import com.google.common.base.Predicate;
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;
Review Comment:
```
/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:
https://github.com/apache/pulsar/blob/e73b29863614a210d14e105c26f691c29d0548d5/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L558-L563
---
moderinzer use ASM to scan the class files and detect the latency usage.
current rule only detect `implementing legacy interfaces`, see https://github.com/gaul/modernizer-maven-plugin/issues/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`):
```java
private final Map<String, Violation> violations;
...
Violation violation = violations.get(token);
checkToken(token, violation, name, lineNumber);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] MarvinCai commented on pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on PR #17691:
URL: https://github.com/apache/pulsar/pull/17691#issuecomment-1260323765
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] youzipi commented on pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17691:
URL: https://github.com/apache/pulsar/pull/17691#issuecomment-1259242887
@MarvinCai @codelipenghui
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
lhotari merged PR #17691:
URL: https://github.com/apache/pulsar/pull/17691
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17691:
URL: https://github.com/apache/pulsar/pull/17691#discussion_r978340745
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java:
##########
@@ -18,13 +18,13 @@
*/
package org.apache.bookkeeper.mledger;
-import com.google.common.base.Predicate;
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;
Review Comment:
Interesting. As we enable Modernizer for managed-ledger already, doesn't this file cause failure already?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] youzipi commented on pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17691:
URL: https://github.com/apache/pulsar/pull/17691#issuecomment-1255843950
@MarvinCai @tisonkun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui closed pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
codelipenghui closed pull request #17691: [cleanup][broker][Modernizer] fix violations in pulsar-broker
URL: https://github.com/apache/pulsar/pull/17691
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org