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/08 11:56:56 UTC

[GitHub] [pulsar] youzipi opened a new pull request, #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

youzipi opened a new pull request, #16993:
URL: https://github.com/apache/pulsar/pull/16993

   Master Issue: #12271 #16964
   
   ### Motivation
   
   Apply Maven Modernizer plugin to enforce we move away from legacy APIs.
   
   ### Modifications
   
   Add Maven Modernizer plugin in pulsar-broker module and fix violation.
   
   ### 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] tisonkun commented on pull request #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1219102306

   @youzipi It seems one required test failed. Please merge master and push for another run.


-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1214921619

   /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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1219397875

   /pulsarbot run Pulsar CI / CI - Unit - Brokers - Broker Group 2 (pull_request)
   


-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1208108844

   Related to #16991 ?


-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1220174102

   Thanks for your contribution @youzip! Go ahead.


-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1208111758

   > Related to #16991 ?
   
   yes, I have fix the reference.


-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1219396221

   
   /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] codelipenghui merged pull request #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #16993:
URL: https://github.com/apache/pulsar/pull/16993


-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#discussion_r947376228


##########
pulsar-broker/pom.xml:
##########
@@ -425,6 +425,25 @@
 
   <build>
     <plugins>
+      <plugin>
+        <groupId>org.gaul</groupId>
+        <artifactId>modernizer-maven-plugin</artifactId>
+        <configuration>
+          <failOnViolations>false</failOnViolations>

Review Comment:
   @MarvinCai please check this comment https://github.com/apache/pulsar/pull/16993#discussion_r945935655
   
   I'm OK since this patch does improve the situation.



-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1208964172

   /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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1219450857

   @youzipi https://github.com/apache/pulsar/pull/16993#issuecomment-1219396221 this comment is correct, due to CI traffic pulsarbot may queue for resources to execute. Wait a minute :)


-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#discussion_r941305175


##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java:
##########
@@ -496,7 +497,7 @@ public void testGlobalNamespaceReplicationConfiguration() throws Exception {
         Set<String> repCluster = (Set<String>) asyncRequests(rsp -> namespaces.getNamespaceReplicationClusters(rsp,
                 this.testGlobalNamespaces.get(0).getTenant(), this.testGlobalNamespaces.get(0).getCluster(),
                 this.testGlobalNamespaces.get(0).getLocalName()));
-        assertEquals(repCluster, Sets.newHashSet());
+        assertEquals(repCluster, new HashSet());

Review Comment:
   ```suggestion
           assertEquals(repCluster, new HashSet<>());
   ```



##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/MaxUnackedMessagesTest.java:
##########
@@ -119,7 +119,7 @@ public void testMaxUnackedMessagesOnSubscription() throws Exception {
 
         // (2) try to consume messages: but will be able to consume number of messages = unackMsgAllowed
         Message<?> msg = null;
-        Map<Message<?>, Consumer<?>> messages = Maps.newHashMap();
+        Map<Message<?>, Consumer<?>> messages = new HashMap();

Review Comment:
   ```suggestion
           Map<Message<?>, Consumer<?>> messages = new HashMap<>();
   ```



##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java:
##########
@@ -1230,7 +1231,7 @@ public void testSubscribeRate() throws Exception {
         SubscribeRate subscribeRate = new SubscribeRate(1, 5);
         String namespace = "my-tenants/my-namespace";
         admin.tenants().createTenant("my-tenants",
-                new TenantInfoImpl(Sets.newHashSet(), Sets.newHashSet(testLocalCluster)));
+                new TenantInfoImpl(new HashSet(), Sets.newHashSet(testLocalCluster)));

Review Comment:
   ```suggestion
                   new TenantInfoImpl(new HashSet<>(), Set.of(testLocalCluster)));
   ```



##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java:
##########
@@ -1230,7 +1231,7 @@ public void testSubscribeRate() throws Exception {
         SubscribeRate subscribeRate = new SubscribeRate(1, 5);
         String namespace = "my-tenants/my-namespace";
         admin.tenants().createTenant("my-tenants",
-                new TenantInfoImpl(Sets.newHashSet(), Sets.newHashSet(testLocalCluster)));
+                new TenantInfoImpl(new HashSet(), Sets.newHashSet(testLocalCluster)));
         admin.namespaces().createNamespace(namespace, Sets.newHashSet(testLocalCluster));

Review Comment:
   ```suggestion
           admin.namespaces().createNamespace(namespace, Set.of(testLocalCluster));
   ```



-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
MarvinCai commented on code in PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#discussion_r947368626


##########
pulsar-broker/pom.xml:
##########
@@ -425,6 +425,25 @@
 
   <build>
     <plugins>
+      <plugin>
+        <groupId>org.gaul</groupId>
+        <artifactId>modernizer-maven-plugin</artifactId>
+        <configuration>
+          <failOnViolations>false</failOnViolations>

Review Comment:
   should be true after as we've fixed all the violation?



##########
pulsar-broker/pom.xml:
##########
@@ -425,6 +425,25 @@
 
   <build>
     <plugins>
+      <plugin>
+        <groupId>org.gaul</groupId>
+        <artifactId>modernizer-maven-plugin</artifactId>
+        <configuration>
+          <failOnViolations>false</failOnViolations>

Review Comment:
   should be true as we've fixed all the violation?



-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1219398658

   > /pulsarbot run-failure-checks
   
   how to trigger the `canceled` checks? @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] tisonkun commented on a diff in pull request #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#discussion_r945935655


##########
pulsar-broker/pom.xml:
##########
@@ -425,6 +425,25 @@
 
   <build>
     <plugins>
+      <plugin>
+        <groupId>org.gaul</groupId>
+        <artifactId>modernizer-maven-plugin</artifactId>
+        <configuration>
+          <failOnViolations>false</failOnViolations>

Review Comment:
   Is it possible we turn this option to `true` and exclude classes we don't handle yet?
   
   I'd prefer we enable the checker and gradually remove the exclusion instead of keeping the check dry-run.



-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#discussion_r944092410


##########
pulsar-broker/pom.xml:
##########
@@ -425,6 +425,25 @@
 
   <build>
     <plugins>
+      <plugin>
+        <groupId>org.gaul</groupId>
+        <artifactId>modernizer-maven-plugin</artifactId>
+        <configuration>
+          <failOnViolations>false</failOnViolations>
+          <javaVersion>8</javaVersion>

Review Comment:
   I think broker can use `${pulsar.broker.compiler.release}` which is effectively 17.



-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on code in PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#discussion_r946510405


##########
pulsar-broker/pom.xml:
##########
@@ -425,6 +425,25 @@
 
   <build>
     <plugins>
+      <plugin>
+        <groupId>org.gaul</groupId>
+        <artifactId>modernizer-maven-plugin</artifactId>
+        <configuration>
+          <failOnViolations>false</failOnViolations>

Review Comment:
   it only provides `ignore` packages, i need to list too much packages.
   ```
             <ignorePackages>
               <ignorePackage>org.apache.pulsar.client</ignorePackage>
             </ignorePackages>
   ```
   i will list packages and start migrate the multi small packages in next PR.



-- 
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 #16993: [cleanup][broker] Add Maven Modernizer plugin in pulsar-broker and fix violation in `test: broker/admin`

Posted by GitBox <gi...@apache.org>.
youzipi commented on PR #16993:
URL: https://github.com/apache/pulsar/pull/16993#issuecomment-1209180898

   /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