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/08/25 08:01:33 UTC
[GitHub] [pulsar] youzipi opened a new pull request, #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
youzipi opened a new pull request, #17275:
URL: https://github.com/apache/pulsar/pull/17275
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
- [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 pull request #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1241430228
> 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.
I merge the master today, but is shows only 5 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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1244812618
@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] tisonkun commented on pull request #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1241424647
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.
--
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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1240541468
/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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1240737214
/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] tisonkun commented on pull request #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1241454395
@youzipi this is correct. Other tasks will be scheduled after prerequisites are finished.
--
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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on code in PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#discussion_r971898266
##########
pulsar-broker/pom.xml:
##########
@@ -432,9 +432,15 @@
<failOnViolations>true</failOnViolations>
<javaVersion>17</javaVersion>
<ignorePackages>
- <ignorePackage>org.apache.pulsar.broker</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.admin</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.namespace</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.service</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.stats</ignorePackage>
<ignorePackage>org.apache.pulsar.client</ignorePackage>
</ignorePackages>
+ <exclusionPatterns>
+ <exclusionPattern>java/lang/StringBuffer."<init>":.*</exclusionPattern>
Review Comment:
@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:
https://github.com/apache/pulsar/pull/17275#discussion_r963809645
--
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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on code in PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#discussion_r971898266
##########
pulsar-broker/pom.xml:
##########
@@ -432,9 +432,15 @@
<failOnViolations>true</failOnViolations>
<javaVersion>17</javaVersion>
<ignorePackages>
- <ignorePackage>org.apache.pulsar.broker</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.admin</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.namespace</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.service</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.stats</ignorePackage>
<ignorePackage>org.apache.pulsar.client</ignorePackage>
</ignorePackages>
+ <exclusionPatterns>
+ <exclusionPattern>java/lang/StringBuffer."<init>":.*</exclusionPattern>
Review Comment:
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:
https://github.com/apache/pulsar/pull/17275#discussion_r963809645
--
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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on code in PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#discussion_r963823203
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/intercept/InterceptFilterOutTest.java:
##########
@@ -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();
Review Comment:
yes, I saw the failed case.
I plan to exclude this rule.
I created an issue:
https://github.com/gaul/modernizer-maven-plugin/issues/149
they insist this as a default rule.
--
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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#discussion_r971871452
##########
pulsar-broker/pom.xml:
##########
@@ -432,9 +432,15 @@
<failOnViolations>true</failOnViolations>
<javaVersion>17</javaVersion>
<ignorePackages>
- <ignorePackage>org.apache.pulsar.broker</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.admin</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.namespace</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.service</ignorePackage>
+ <ignorePackage>org.apache.pulsar.broker.stats</ignorePackage>
<ignorePackage>org.apache.pulsar.client</ignorePackage>
</ignorePackages>
+ <exclusionPatterns>
+ <exclusionPattern>java/lang/StringBuffer."<init>":.*</exclusionPattern>
Review Comment:
@youzipi may I ask the reason you add this pattern?
--
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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1247821672
@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 merged pull request #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #17275:
URL: https://github.com/apache/pulsar/pull/17275
--
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 a diff in pull request #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on code in PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#discussion_r963809645
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/intercept/InterceptFilterOutTest.java:
##########
@@ -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();
Review Comment:
method signature is `StringBuffer getRequestURL();`, so can't use StringBuilder here.
--
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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1229733567
/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 #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1241423842
I can't trigger the checks.
because it was cancelled manually?
@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] youzipi commented on pull request #17275: [cleanup][broker][Modernizer] fix violations in pulsar-broker
Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #17275:
URL: https://github.com/apache/pulsar/pull/17275#issuecomment-1231067610
/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