[RIP-37] Add new APIs for producer #3987

aaron-ai · 2022-03-14T09:39:14Z

As we mentioned in RIP-37 New and unified APIs, establish a new APIs specifications. we divide it into two independent pull requests.

The producer part.
The consumer part.

this pull request is about the producer part.

related issue: #3973

aaron-ai · 2022-03-14T09:41:40Z

@zhouxinyu @lizhanhui @lollipopjin @duhenglucky @RongtongJin @drpmma @chenzlalvin

dongeforever

Except for the admin operation, the abilities of the new API should be a superset of the old API.

dongeforever · 2022-03-14T11:10:01Z

apis/src/main/java/org/apache/rocketmq/apis/message/Message.java

+     * @return message group, which is optional.
+     */
+    Optional<String> getMessageGroup();
+


This interface need a method getMessageQueue() too.
It may need to send message to the specified MessageQueue.
The case cannot be covered by getMessageGroup().

The priority of getMessageQueue() is bigger than getMessageGroup().

Message here is for producer, user may build message before sending, so Message#getMessageQeue may be not appropriate, but we may consider to add it into the return value of producer sending method.

I repeat this:
Except for the admin operation, the abilities of the new API should be a superset of the old API.

The old API could send messages to the specified message queue?

So how to do it in the new API?

the abilities of the new API should be a superset of the old API

We don't follow this rule, many useless or low-frequency abilities will be removed in the new APIs, the new APIs don't expand the old APIs.

As for sending messages to a specific queue, we were going to add it in the future version. Consider MessageQueue is a major model of RocketMQ, we may need to add it in the first version. But, I am not sure it's the right choice, hope more contributors could express opinions on this topic, especially from user perspectives.

dongeforever · 2022-03-14T11:15:15Z

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java

+     * @throws PersistenceException              if encountered persistence failure from server.
+     * @throws TransactionCheckerNotSetException if {@link TransactionChecker} is not set.
+     */
+    MessageId send(Message message, Transaction transaction) throws ClientException;


This is not the two-phase API!

How about like this :
try {
TransactionMark mark = producer.prepareSend(Message message);
//do process
producer.commit(mark);
} catch(Exception e) {
producer.rollback(mark);
}

Such style is easy to be integrated with db transaction. And the code will not be split.

Well, we may send more than one message in single transaction.

How to send more than one message using such API?
MessageId send(Message message, Transaction transaction)

The old API for the transaction is not naturally two-phase based. It's better to polish it in the new API.
But currently, the new API is just like the old

How to send more than one message using such API? MessageId send(Message message, Transaction transaction)

The old API for the transaction is not naturally two-phase based. It's better to polish it in the new API. But currently, the new API is just like the old

The code example has been added to the javadocs.

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java

dongeforever · 2022-03-14T11:23:51Z

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java

+     * @throws NetworkConnectionException       if there is a network connection problem.
+     * @throws PersistenceException             if encountered persistence failure from server.
+     */
+    MessageId send(Message message) throws ClientException;


Where is the SendResult?

The client at least needs to know the offset and messagequeue for checking!

SendResult is not essential, because exception may be a better way to indicate the result, but offset and message queue may be considered.

apis/src/main/java/org/apache/rocketmq/apis/message/MessageView.java

apis/src/main/java/org/apache/rocketmq/apis/message/Message.java

apis/src/main/java/org/apache/rocketmq/apis/message/MessageIdVersion.java

apis/src/main/java/org/apache/rocketmq/apis/exception/AuthorisationException.java

apis/src/main/java/org/apache/rocketmq/apis/exception/ClientException.java

lifepuzzlefun · 2022-03-15T05:12:03Z

apis/src/main/java/org/apache/rocketmq/apis/message/MessageBuilder.java

+     * @param topic topic for the message.
+     * @return the message builder instance.
+     */
+    MessageBuilder setTopic(String topic);


if we remove set prefix the api will be more clean to the developer.

Well, setter/getter style is more common in java, MessageBuilder#topic() or MessageBuilder#topic(string topicName) may be more common in cpp.

lifepuzzlefun · 2022-03-15T05:14:45Z

apis/src/main/java/org/apache/rocketmq/apis/message/MessageId.java

+     *
+     * @return string-formed string id.
+     */
+    String toString();


this name is the same as the java origin Object.toString. i think we can change the method name. the origin is anti-parttern naming.

#toString() here aims to emphasize that the implementation must override this method, maybe there is a better solution?

lifepuzzlefun · 2022-03-15T05:17:52Z

apis/src/main/java/org/apache/rocketmq/apis/message/MessageView.java

+     *
+     * @return the <strong>deep copy</strong> of message body.
+     */
+    byte[] getBody();


deep copy seems will harm the performance. but there seems no better return type here.

how about ByteBuffer here which can be set as unmodifable

how about ByteBuffer here which can be set as unmodifable

Nice suggestion.

lifepuzzlefun · 2022-03-15T05:22:38Z

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java

+     * @throws TransactionCheckerNotSetException if {@link TransactionChecker} is not set.
+     * @throws ProducerClosedAlreadyException    if producer is closed already.
+     */
+    Transaction beginTransaction() throws ClientException;


maybe an interface extand producer add some Transaction relate method will be better.

Could you please provide some code samples? @WJL3333

lifepuzzlefun · 2022-03-15T05:22:53Z

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java

+     */
+    @SuppressWarnings("UnstableApiUsage")
+    @Override
+    void close();


will close throw exception here?

lifepuzzlefun · 2022-03-15T05:24:04Z

apis/src/main/java/org/apache/rocketmq/apis/producer/ProducerBuilder.java

+     * @param clientConfiguration client's configuration.
+     * @return the producer builder instance.
+     */
+    ProducerBuilder setClientConfiguration(ClientConfiguration clientConfiguration);


same as Messagebuilder. no set prefix will make api more clean in fluent api call.

We can find many fluent builder samples in JDK which use the setter pattern, such as https://docs.oracle.com/javase/7/docs/api/java/util/Locale.Builder.html

@WJL3333 Lombok doesn't support prefixed builders at the beginning, but this project resolved this issue later, please refer to projectlombok/lombok#1805 (comment), there are many reasons support we use prefixed builders.

lifepuzzlefun · 2022-03-15T05:25:42Z

apis/src/main/java/org/apache/rocketmq/apis/producer/ProducerBuilder.java

+     * @throws NetworkTimeoutException    if encountered network timeout to communicate with server.
+     * @throws NetworkConnectionException if there is a network connection problem.
+     */
+    Producer start() throws ClientException;


may be move start to the producer.
some case will just build the producer without start.

Actually producerBuilder#start could avoid to start producer repeatedly. What's more, a producer which is not started does not make sense.

It's strange if we don't have a build method in a builder interface.

If we only provide a start method in the producer builder, that means all the producers are started by default, so why we should have a start?

It's strange if we don't have a build method in a builder interface.

If we only provide a start method in the producer builder, that means all the producers are started by default, so why we should have a start?

I agree with the opinon that lack of #build() may be a little bit weird, let's look at it another way. Is it unnecessary for the user to start producer manually? If so, then I think it's just a naming issue.

lifepuzzlefun · 2022-03-15T05:26:04Z

apis/src/main/java/org/apache/rocketmq/apis/producer/Transaction.java

+     * <strong>actually we omit the exception on purpose because {@link TransactionChecker} is the unique right way
+     * to solve the suspended transaction rather than commit or roll-back repeatedly.</strong>
+     */
+    void commit();


will commit throw exception need the caller process?

TransactionChecker is less real-time, which may result in high latency when the commit failed.

apis/src/main/java/org/apache/rocketmq/apis/producer/ProducerBuilder.java

zhouxinyu · 2022-03-15T10:28:48Z

apis/src/main/java/org/apache/rocketmq/apis/producer/ProducerBuilder.java

+/**
+ * Builder to config and start {@link Producer}.
+ */
+public interface ProducerBuilder {


Do we have a chance to force our developers to set the required fields when building the Producer or Message?

Builder should throw null field check exception when call build interface.

apis/src/main/java/org/apache/rocketmq/apis/message/Message.java

zhouxinyu · 2022-03-15T12:22:10Z

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java

+     * @throws PersistenceException              if encountered persistence failure from server.
+     * @throws TransactionCheckerNotSetException if {@link TransactionChecker} is not set.
+     */
+    MessageView send(Message message, Transaction transaction) throws ClientException;


Tons of exceptions are thrown, it's daunting.

I suggest we only declare a ClientException with managed code, request-id, message, etc.

These exceptions are unchecked, which means we don't want our users to use these exceptions to control their logic, so a simple ClientException is enough.

These exceptions are unchecked, which means we don't want our users to use these exceptions to control their logic, so a simple ClientException is enough.

First of all, exceptions are inevitable. In fact, about the issue that whether to use checked exception or not, maybe no need to prevent users to do responding operation by the exception thrown deliberately? For example, users just want to count how many times the network timeout exception occurred.

In addition, about the issue to use error codes or specific exception, actuall they can all solve the problem in theory, here's my concern

It is very easy to abuse error code.

Not all exceptions will have the same parameters, such as requestId, they may only exist in some exceptions.

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java

coveralls · 2022-03-17T12:35:23Z

Coverage decreased (-0.08%) to 49.592% when pulling d42aa99 on aaron-ai:producer_pr into 176e0d5 on apache:5.0.0-beta.

coveralls · 2022-03-17T12:35:24Z

Coverage decreased (-0.008%) to 49.654% when pulling d30f3f5 on aaron-ai:producer_pr into 0b7291b on apache:5.0.0-beta.

This reverts commit 8fc952d.

RongtongJin changed the title ~~Add new APIs for producer~~ [RIP-37] Add new APIs for producer Mar 14, 2022

aaron-ai force-pushed the producer_pr branch from a5bfe60 to 643e8a3 Compare March 14, 2022 10:05

dongeforever reviewed Mar 14, 2022

View reviewed changes

RongtongJin reviewed Mar 14, 2022

View reviewed changes

aaron-ai force-pushed the producer_pr branch 2 times, most recently from cdd3362 to bc2a1b9 Compare March 14, 2022 12:00

aaron-ai requested a review from RongtongJin March 14, 2022 12:05

aaron-ai force-pushed the producer_pr branch 2 times, most recently from e06beb9 to 54fa721 Compare March 14, 2022 12:47

lifepuzzlefun reviewed Mar 15, 2022

View reviewed changes

apis/src/main/java/org/apache/rocketmq/apis/exception/AuthorisationException.java Show resolved Hide resolved

lifepuzzlefun reviewed Mar 15, 2022

View reviewed changes

aaron-ai force-pushed the producer_pr branch 2 times, most recently from e36e9ef to 62a1f28 Compare March 15, 2022 09:18

zhouxinyu reviewed Mar 15, 2022

View reviewed changes

hill007299 reviewed Mar 15, 2022

View reviewed changes

apis/src/main/java/org/apache/rocketmq/apis/message/Message.java Show resolved Hide resolved

zhouxinyu reviewed Mar 15, 2022

View reviewed changes

aaron-ai force-pushed the producer_pr branch from 62a1f28 to 238df75 Compare March 15, 2022 14:16

aaron-ai requested review from zhouxinyu and hill007299 March 15, 2022 14:52

aaron-ai force-pushed the producer_pr branch 8 times, most recently from 0f87eb2 to 2d276ba Compare March 17, 2022 06:05

zhouxinyu requested changes Mar 17, 2022

View reviewed changes

apis/src/main/java/org/apache/rocketmq/apis/producer/Producer.java Outdated Show resolved Hide resolved

aaron-ai force-pushed the producer_pr branch from 2d276ba to 622c126 Compare March 17, 2022 10:58

aaron-ai force-pushed the producer_pr branch from 622c126 to d42aa99 Compare March 17, 2022 11:14

aaron-ai force-pushed the producer_pr branch 2 times, most recently from cbcb38c to fc90eac Compare March 17, 2022 13:46

aaron-ai requested a review from zhouxinyu March 17, 2022 16:20

aaron-ai force-pushed the producer_pr branch 2 times, most recently from c529eec to 23fa2c2 Compare March 18, 2022 02:10

Add new APIs for producer

d30f3f5

aaron-ai force-pushed the producer_pr branch from 23fa2c2 to d30f3f5 Compare March 18, 2022 03:07

lizhanhui merged commit 8fc952d into apache:5.0.0-beta Mar 18, 2022

chenzlalvin mentioned this pull request Mar 22, 2022

[RIP-37] Add new APIs for consumer #4019

Merged

MatrixHB mentioned this pull request Apr 6, 2022

ADD convient method in MQProducer which return CompletableFuture<SendResult>(添加返回CompletableFuture<SendResult>的异步发送方法) #4120

Closed

aaron-ai added a commit to aaron-ai/rocketmq that referenced this pull request Jun 20, 2022

Revert "Add new APIs for producer (apache#3987)"

170fc7e

This reverts commit 8fc952d.

zhouxinyu pushed a commit that referenced this pull request Jun 20, 2022

Revert "Add new APIs for producer (#3987)"

96e9e48

This reverts commit 8fc952d.

[RIP-37] Add new APIs for producer #3987

[RIP-37] Add new APIs for producer #3987

Conversation

aaron-ai commented Mar 14, 2022

aaron-ai commented Mar 14, 2022 • edited Loading

dongeforever 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-ai Mar 15, 2022 • 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

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

aaron-ai Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-ai Mar 15, 2022 • 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

aaron-ai Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

coveralls commented Mar 17, 2022

coveralls commented Mar 17, 2022 • edited Loading

aaron-ai commented Mar 14, 2022 •

edited

Loading

aaron-ai Mar 15, 2022 •

edited

Loading

aaron-ai Mar 15, 2022 •

edited

Loading

aaron-ai Mar 15, 2022 •

edited

Loading

aaron-ai Mar 15, 2022 •

edited

Loading

coveralls commented Mar 17, 2022 •

edited

Loading