-
Notifications
You must be signed in to change notification settings - Fork 641
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KREST-10313 Prototype of batch REST Produce #1162
Conversation
kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/ProduceBatchAction.java
Outdated
Show resolved
Hide resolved
kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/ProduceBatchAction.java
Show resolved
Hide resolved
kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/ProduceBatchAction.java
Outdated
Show resolved
Hide resolved
kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/ProduceBatchAction.java
Show resolved
Hide resolved
@@ -2330,6 +2424,44 @@ components: | |||
nullable: true | |||
nullable: true | |||
|
|||
ProduceBatchRequest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do run it through various open-api linters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will do when I'm ready.
ProduceBatchResponse: | ||
description: |- | ||
The response containing a delivery report for each record produced to a topic. | ||
A separate delivery report will be returned, in the same order, each with its own error_code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it should mention a high level list of "error_codes" to expect in a delivery report.
kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/ProduceBatchAction.java
Show resolved
Hide resolved
kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/ProduceBatchAction.java
Show resolved
Hide resolved
executorService); | ||
} | ||
|
||
private CompletableFuture<ProduceBatchResultEntry> produce( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to better organize this code? 🤔
with 2 ProduceAction*.java files, any future modifications like to rate-limiting, billing would have to be in 2 places, not ideal.
Maybe divide the logic into 2 bits -
- "internal-facing-logic" - this will the common produce bits, like "actual produce", rate-limiting, billing etc.
- "external-facing" Produce*Action.java that validations, ser & deser for external data-strucutures.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably ripe for refactoring when it's settled down a bit.
kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/ProduceBatchAction.java
Show resolved
Hide resolved
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
public class ProduceBatchActionTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very thorough UTs, nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than few open comments, I think its lgtm to merge, so approving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
An alternative to streaming REST Produce based on an array of requests and responses.