PIP-12 Introduce builder for creating Producer Consumer Reader #1089

merlimat · 2018-01-22T22:16:43Z

Motivation

Full context at
https://github.com/apache/incubator-pulsar/wiki/PIP-12:-Introduce-builder-for-creating-Producer-Consumer-Reader

Modifications

Added new builder methods

sijie · 2018-02-01T06:06:34Z

/cc @zhaijack @mgodave

sijie · 2018-02-01T06:06:45Z

/cc @lucperkins

sijie

the proposed api looks good me. but it seems some of the javadocs were copied from configuration based methods, we need to update the javadoc for those methods.

sijie · 2018-02-01T07:35:59Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java

+     *
+     * <pre>
+     * <code>
+     * ClientConfiguration conf = new ClientConfiguration();


documentation seems need to be updated

Sure, this was just a placeholder. I'll clean up all Javadocs when submitting final PR.

zhaijack · 2018-02-01T11:40:42Z

+1, LGTM, currently.
Once we have subscription for topics and pattern, we need add them in.

mgodave · 2018-02-01T17:35:46Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ProducerBuilder.java

+
+public interface ProducerBuilder {
+
+    Producer create() throws PulsarClientException;


nit: since this is a builder, do we want to go with the canonical 'build' terminology instead?

Yes, I was debating about this. My reasoning here was to differentiate the actions:

client.newProducer().topic(MY_TOPIC).create(); // versus client.newConsumer().topic(MY_TOPIC).subscriptionName(MY_SUB).subscribe();

Subscribe here implies to create a durable Subscription on the topic, not just building an ephemeral Consumer instance.

I'm not 100% settled on this. Please anyone share opinions ;)

mgodave · 2018-02-01T17:36:57Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ProducerBuilder.java

+
+    Producer create() throws PulsarClientException;
+
+    CompletableFuture<Producer> createAsync();


I feel like having async and sync versions cloud things. The nature of the library is that it is async and if all we are doing is calling CompletableFuture::get and catching exceptions for the user, is it really worth it?

I think most users still reasons in term of "sync" API, especially for creating the producer/consumer (compared to publishing messages).

merlimat · 2018-02-08T23:04:24Z

The other doubt here was which form would be preferred between:

Producer producer = client.newProducer().topic("my-topic").create();

versus

Producer producer = client.newProducer("my-topic").create();

This is regarding required and optional parameters.

The 2nd is a bit more concise, though that doesn't mean a lot if you're setting multiple options anyway.

Opinions with respect to this?

merlimat · 2018-02-14T06:01:10Z

This is ready for review. Once this is finalized and merged, I'll start converting all the code to use new API.

merlimat · 2018-02-14T06:04:09Z

Once we have subscription for topics and pattern, we need add them in.

Yes, the idea was to have different ways to specify:

// Single topic
client.newConsumer().topic(MY_TOPIC).subscriptioName(SUB).build();

// List of topics
List<String> myList = ...;
client.newConsumer().topics(myList).subscriptioName(SUB).build();

// Regex
client.newConsumer().topicsRegex("test.*").subscriptioName(SUB).build();

sijie

the change looks good to me. some minor comments.

it is a really awesome change to move the api to builder pattern!

sijie · 2018-02-14T16:24:53Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java

+import org.apache.pulsar.client.api.PulsarClientException.UnsupportedAuthenticationException;
+
+/**
+ * Builder interface that is used to construct a {@link PulsarClient} instance.


nit: it might be good to add "@SInCE 2.0". so we can track when we introduce the build methods.

Sure, will add @since

sijie · 2018-02-14T16:25:37Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java

+     * PulsarClient client2 = builder.clone().serviceUrl(URL_2).build();
+     * </pre>
+     */
+    ClientBuilder clone();


nit: @Override?

clone() is a bit weird in the way it's treated. It cannot be marked as @Override

sijie · 2018-02-14T16:27:05Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java

+     * Finalize the consumer creation by subscribing to the topic.
+     *
+     * <p>
+     * Subscribing to the c


incomplete comment?

ups, forgot to finish

sijie · 2018-02-14T16:27:39Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java

+     * <p>
+     * Subscribing to the c
+     *
+     * @return


@return a consumer?

sijie · 2018-02-14T16:30:59Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ReaderConfiguration.java

-     * 
-     * @param The consumer action
+     *
+     * @param The


the param field seems to be wrong?

maskit · 2018-02-15T14:36:19Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java

+     *
+     * @param topicName
+     */
+    ConsumerBuilder topic(String topicName);


It's just an idea but we may want to pass a Topic object instead of its string representation. Then this interface would be freed to define a format of topic name string (persistent://.../.../...), and users can manage their topic names in any format.

I would leave it as a string, at least for now, because that would be a bigger change I think. We're already trying to address the long topic names by having some degree of implicitenes. See https://github.com/apache/incubator-pulsar/wiki/PIP-10:-Remove-cluster-for-namespace-and-topic-names and https://github.com/apache/incubator-pulsar/wiki/PIP-11:-Short-topic-names

Alright. Yeah, and the change is not necessary for introducing builders.

Below is what I thought.

We should probably introduce a parser for topic name string to reduce duplicate parsing code. Otherwise PIP-10 and PIP-11 would be tough because we need to modify all parsing code here and there. If we introduce the parser, the builder implementation would be like:

public ConsumerBuilder topic(String topicName) { this.topic = Topic.parse(topicName); return this; }

The Topic class would be like this:

class Topic { public Topic(String topicName) { // parse ... this.namespace = .... this.cluster = ... ... } public getNamespace() { return this.namespace; } ... }

Once we did this, other components that use Topic object wouldn't need to care how the object was created. And if so, it might be useful if users could create own Topic implementation and pass it to builders.

So I'm not saying we should eliminate the Pulsar topic URL format. I just thought using Topic object may increase its capability.

For parsing, it's already all concentrated in the DestinationName class, which I plan to rename into TopicName. It is not exposed to users though.

maskit · 2018-02-15T14:41:27Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java

+     *
+     * @param tlsAllowInsecureConnection
+     */
+    ClientBuilder tlsAllowInsecureConnection(boolean tlsAllowInsecureConnection);


How about "allowTlsInsecureConnection"?

Sure, with the builder it makes sense a more "imperative" style

maskit · 2018-02-15T14:43:20Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java

+     *
+     * @param enableTLS
+     */
+    ClientBuilder enableTLS(boolean enableTLS);


"TLS"? or "Tls"? Need consistency.

Right, I'll go back back to "Tls"

maskit · 2018-02-15T14:47:06Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ProducerConfiguration.java

@@ -377,7 +377,8 @@ public void removeEncryptionKey(String key) {
    /**
     * Sets the ProducerCryptoFailureAction to the value specified
     *
-     * @param The producer action
+     * @param The


missing a parameter name

merlimat · 2018-02-16T23:55:40Z

retest this please

maskit

👍

merlimat · 2018-02-17T17:04:43Z

Rebased to fix tls test certificates issue

Since the apache#1089，the interface 'org.apache.pulsar.client.api.PulsarClient' has supported builder for consumer and producer. And,int the apache#3272, some deprecated client API have been removed including 'public static PulsarClient create(String serviceUrl)','Consumer<byte[]> subscribe(String topic, String subscription)' and 'Producer<byte[]> createProducer(String topic)'. So, I modified the example with the current version of the interface.

* Update concepts-messaging.md Since the #1089，the interface 'org.apache.pulsar.client.api.PulsarClient' has supported builder for consumer and producer. And,int the #3272, some deprecated client API have been removed including 'public static PulsarClient create(String serviceUrl)','Consumer<byte[]> subscribe(String topic, String subscription)' and 'Producer<byte[]> createProducer(String topic)'. So, I modified the example with the current version of the interface. * Apply suggestions from code review Specify message schema Co-Authored-By: Matteo Merli <mmerli@apache.org>

merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Jan 22, 2018

merlimat added this to the 2.0.0-incubating milestone Jan 22, 2018

merlimat self-assigned this Jan 22, 2018

merlimat requested review from sijie, rdhabalia, jai1, saandrews, nkurihar and yush1ga January 22, 2018 22:16

merlimat force-pushed the builder-api branch from 873c90a to f5ee1fe Compare January 22, 2018 22:44

sijie reviewed Feb 1, 2018

View reviewed changes

mgodave approved these changes Feb 1, 2018

View reviewed changes

merlimat mentioned this pull request Feb 9, 2018

PIP-13-1/3: Provide TopicsConsumer to consume from several topics under same namespace #1103

Merged

merlimat force-pushed the builder-api branch from f5ee1fe to f17cac1 Compare February 14, 2018 05:55

merlimat changed the title ~~WIP - PIP-12 Introduce builder for creating Producer Consumer Reader~~ PIP-12 Introduce builder for creating Producer Consumer Reader Feb 14, 2018

zhaijack mentioned this pull request Feb 14, 2018

Make subscribe for TopicsConsumer using builder #1237

Closed

mgodave mentioned this pull request Feb 14, 2018

Schema registry #1137

Closed

sijie approved these changes Feb 14, 2018

View reviewed changes

merlimat requested review from maskit and sschepens February 14, 2018 17:42

maskit reviewed Feb 15, 2018

View reviewed changes

maskit approved these changes Feb 17, 2018

View reviewed changes

merlimat force-pushed the builder-api branch from 585dd9b to 03dcd22 Compare February 17, 2018 17:04

merlimat added 3 commits February 17, 2018 09:04

PIP-12: Introduce builders for creating Producer Consumer Reader

769dcc3

Fixed Javadocs

5742ca8

Addressed comments

03dcd22

merlimat merged commit cb5eb5b into apache:master Feb 17, 2018

merlimat deleted the builder-api branch February 17, 2018 20:22

This was referenced Feb 23, 2018

Refactored ClientConfuguration to use ClientConfigurationData shared with ClientBuilderImpl #1276

Merged

Converted main part of code to use builder APIs with typed interface #1311

Merged

hapihu mentioned this pull request Jun 5, 2019

Update concepts-messaging.md #4483

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-12 Introduce builder for creating Producer Consumer Reader #1089

PIP-12 Introduce builder for creating Producer Consumer Reader #1089

merlimat commented Jan 22, 2018

sijie commented Feb 1, 2018

sijie commented Feb 1, 2018

sijie left a comment

sijie Feb 1, 2018

merlimat Feb 6, 2018

zhaijack commented Feb 1, 2018

mgodave Feb 1, 2018

merlimat Feb 6, 2018

mgodave Feb 1, 2018

merlimat Feb 6, 2018

merlimat commented Feb 8, 2018

merlimat commented Feb 14, 2018

merlimat commented Feb 14, 2018 •

edited

Loading

sijie left a comment

sijie Feb 14, 2018

merlimat Feb 14, 2018

sijie Feb 14, 2018

merlimat Feb 14, 2018

sijie Feb 14, 2018

merlimat Feb 14, 2018

sijie Feb 14, 2018

sijie Feb 14, 2018

maskit Feb 15, 2018

merlimat Feb 16, 2018

maskit Feb 17, 2018 •

edited

Loading

merlimat Feb 17, 2018

maskit Feb 15, 2018

merlimat Feb 15, 2018

maskit Feb 15, 2018

merlimat Feb 15, 2018

maskit Feb 15, 2018

merlimat commented Feb 16, 2018

maskit left a comment

merlimat commented Feb 17, 2018


		public interface ProducerBuilder {

		Producer create() throws PulsarClientException;


		Producer create() throws PulsarClientException;

		CompletableFuture<Producer> createAsync();

PIP-12 Introduce builder for creating Producer Consumer Reader #1089

PIP-12 Introduce builder for creating Producer Consumer Reader #1089

Conversation

merlimat commented Jan 22, 2018

Motivation

Modifications

sijie commented Feb 1, 2018

sijie commented Feb 1, 2018

sijie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaijack commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Feb 8, 2018

merlimat commented Feb 14, 2018

merlimat commented Feb 14, 2018 • edited Loading

sijie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maskit Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Feb 16, 2018

maskit left a comment

Choose a reason for hiding this comment

merlimat commented Feb 17, 2018

merlimat commented Feb 14, 2018 •

edited

Loading

maskit Feb 17, 2018 •

edited

Loading