You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/04/12 06:50:26 UTC

[GitHub] [flink] syhily opened a new pull request, #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

syhily opened a new pull request, #19430:
URL: https://github.com/apache/flink/pull/19430

   This is a backport PR for #19285.


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] syhily commented on a diff in pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
syhily commented on code in PR #19430:
URL: https://github.com/apache/flink/pull/19430#discussion_r861027941


##########
flink-connectors/flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/common/config/PulsarClientFactory.java:
##########
@@ -153,7 +153,7 @@ public static PulsarClient createClient(PulsarConfiguration configuration) {
 
     /**
      * PulsarAdmin shares almost the same configuration with PulsarClient, but we separate this
-     * create method for directly creating it.
+     * creating method for directly use it.

Review Comment:
   OK.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] AHeise commented on pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
AHeise commented on PR #19430:
URL: https://github.com/apache/flink/pull/19430#issuecomment-1111803212

   > Since this PR fixes an actual bug, I would expect to add a test case. Can you add one so that the issue does not arise again?
   
   I think this is hard to test on integration level (you probably need to check on Pulsar server if a specific producer is registered) and the change is rather implicitly tested with other tests.
   However, if there is an easy way to test, I'm also in favor of adding it.


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] MartijnVisser merged pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
MartijnVisser merged PR #19430:
URL: https://github.com/apache/flink/pull/19430


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] fapaul commented on a diff in pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
fapaul commented on code in PR #19430:
URL: https://github.com/apache/flink/pull/19430#discussion_r859938837


##########
flink-connectors/flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/common/config/PulsarClientFactory.java:
##########
@@ -153,7 +153,7 @@ public static PulsarClient createClient(PulsarConfiguration configuration) {
 
     /**
      * PulsarAdmin shares almost the same configuration with PulsarClient, but we separate this
-     * create method for directly creating it.
+     * creating method for directly use it.

Review Comment:
   Nit: I am having a hard time understanding this doc string. Can you maybe rephrase it?



##########
flink-connectors/flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/sink/config/PulsarSinkConfigUtils.java:
##########
@@ -70,7 +71,10 @@ public static <T> ProducerBuilder<T> createProducerBuilder(
             PulsarClient client, Schema<T> schema, SinkConfiguration configuration) {
         ProducerBuilder<T> builder = client.newProducer(schema);
 
-        configuration.useOption(PULSAR_PRODUCER_NAME, builder::producerName);
+        configuration.useOption(

Review Comment:
   What happens during an application restart? I guess a new producerName is generated. Is this a problem?



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] syhily commented on a diff in pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
syhily commented on code in PR #19430:
URL: https://github.com/apache/flink/pull/19430#discussion_r861034376


##########
flink-connectors/flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/sink/config/PulsarSinkConfigUtils.java:
##########
@@ -70,7 +71,10 @@ public static <T> ProducerBuilder<T> createProducerBuilder(
             PulsarClient client, Schema<T> schema, SinkConfiguration configuration) {
         ProducerBuilder<T> builder = client.newProducer(schema);
 
-        configuration.useOption(PULSAR_PRODUCER_NAME, builder::producerName);
+        configuration.useOption(

Review Comment:
   This wouldn't be a problem. Because we promise consistency by using transactions.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] flinkbot commented on pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19430:
URL: https://github.com/apache/flink/pull/19430#issuecomment-1096192959

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "7fe3b4d67587ab503212ec4a79ad30272900c829",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "7fe3b4d67587ab503212ec4a79ad30272900c829",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * 7fe3b4d67587ab503212ec4a79ad30272900c829 UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run azure` re-run the last Azure build
   </details>


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] syhily commented on pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
syhily commented on PR #19430:
URL: https://github.com/apache/flink/pull/19430#issuecomment-1099953810

   @flinkbot run azure


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] syhily commented on pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
syhily commented on PR #19430:
URL: https://github.com/apache/flink/pull/19430#issuecomment-1099953862

   @AHeise This is a backport PR. Can you review it?


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink] syhily commented on pull request #19430: [BK-1.15][FLINK-26931][Connector/pulsar] Make the producer name and consumer name unique in Pulsar

Posted by GitBox <gi...@apache.org>.
syhily commented on PR #19430:
URL: https://github.com/apache/flink/pull/19430#issuecomment-1112349280

   > Since this PR fixes an actual bug, I would expect to add a test case. Can you add one so that the issue does not arise again?
   
   I think this could be hard to test. Because we just use the same producer name which is not allowed on the Pulsar. We just need to make sure it was unique.


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org