KAFKA-2071: Replace Producer Request/Response with their org.apache.kafka.common.requests equivalents by dajac · Pull Request #110 · apache/kafka

dajac · 2015-08-04T19:14:09Z

This PR replaces all occurrences of kafka.api.ProducerRequest/ProducerResponse by their common equivalents.

asfbot · 2015-08-04T20:41:46Z

kafka-trunk-git-pr #83 SUCCESS
This pull request looks good

asfbot · 2015-08-11T05:49:19Z

kafka-trunk-git-pr #124 SUCCESS
This pull request looks good

dajac · 2015-08-15T09:27:33Z

@ijuma Ok, thanks. I'll update that.

dajac · 2015-08-17T18:53:53Z

I've rebased it and removed implicit conversion of collections.

asfbot · 2015-08-18T12:05:19Z

kafka-trunk-git-pr #164 SUCCESS
This pull request looks good

dajac · 2015-08-25T05:35:23Z

Merged trunk and fixed conflicts.

asfbot · 2015-08-25T05:39:41Z

kafka-trunk-git-pr #210 FAILURE
Looks like there's a problem with this pull request

dajac · 2015-08-25T05:41:41Z

Failure is not related:

kafka.consumer.MetricsTest > testMetricsLeak FAILED

asfbot · 2015-08-26T06:30:21Z

kafka-trunk-git-pr #221 FAILURE
Looks like there's a problem with this pull request

dajac · 2015-08-26T15:52:06Z

Could someone review this one? May be @gwenshap or @ijuma?

granthenke · 2015-12-11T20:38:30Z

@dajac Are you interested in rebasing this?

dajac · 2015-12-18T13:28:15Z

@granthenke Yes, I'll have some time next week to do it.

dajac · 2015-12-30T16:11:09Z

@granthenke I have rebased this PR. Note that this PR migrates the server and the producer.

dajac · 2015-12-30T16:25:38Z

Migration of the producer is quite straightforward as it mainly happens in SyncProducer and DefaultEventHandler but I'm wondering if SyncProducer is considered as a public API. Is it?

granthenke · 2016-01-06T20:45:42Z

@dajac It looks like the client code changes are not backward compatible. I think this patch either needs to be server side only, or if the clients are going to be updated, they need to be done so in a backward compatible way.

I could be wrong, as we have not had the discussion yet on the mailing list, but I think the idea is to deprecate the older Scala producers and consumers. I am not sure if its worth the effort of updating the client code. (context can be found in the comments on KAFKA-2982)

ijuma · 2016-01-06T21:31:51Z

Looking at the JIRA for this PR, it looks like @gwenshap suggested that the JIRA was about removing k.a.Producer{Request, Response} altogether and that is why @dajac did this. But the producer is indeed public API and it is not yet deprecated so I think we can't do that for the upcoming release.

My suggestion is to only update the server-side and leave the soon to be deprecated clients alone (as @granthenke has suggested too, if I understand correctly).

dajac · 2016-01-07T06:38:51Z

@granthenke @ijuma Indeed, the SyncProducer part is not backward compatible. Making it backward compatible requires to keep the old produce request so it doesn't make sense to upgrade it to the new one. As you both suggested, it is better to not upgrade it.

I'll update the PR to reflect this.

dajac · 2016-01-07T18:16:39Z

@granthenke @ijuma I have rebased and updated it. Could you review it when you have time? All tests pass locally.

granthenke · 2016-01-07T18:34:48Z

Think this is likely just a rebase thing, but recently this was moved above the debug message. Not sure why, but can we keep it that way?

Actually just noticed its there, twice. We should remove this one.

Indeed, I have forgotten to remove it during the rebase. I'll remove it.

I moved it above the debug log in the other PR to make the side-effect more prominent (instead of hidden after a long debug statement).

granthenke · 2016-01-07T18:43:52Z

Thanks for the update. I did a quick visual check and will follow up with a more in depth review likely tomorrow.

ijuma · 2016-01-08T13:07:21Z

I know this name was used in the old ProduceRequest, but it seems a bit ambiguous to me. Something with clear in the name would be clearer for me (clearData, clearRecords, clearPartitionRecords, etc.).

Good point. I'll use clearPartitionRecords.

ijuma · 2016-01-08T13:09:04Z

Nitpick: You don't need () in sizeOf.

ijuma · 2016-01-08T13:25:00Z

I had a look and left some comments. Looks good generally.

dajac · 2016-01-08T17:31:50Z

@granthenke @ijuma I did a first pass and addressed all your reviews. Thanks for you input.

granthenke · 2016-01-13T21:09:14Z

Can use exceptionName method from #754

granthenke · 2016-01-13T21:36:35Z

@dajac Just a minor update for the exceptionName method. Otherwise LGTM.

dajac · 2016-01-14T06:43:59Z

@granthenke I have rebased the PR and addressed your points.

dajac · 2016-01-14T07:40:45Z

@gwenshap @ewencp @junrao Could one of you take a look? @granthenke and @ijuma have reviewed.

ewencp · 2016-01-18T19:25:32Z

Not critical, but could we reuse RequestSend.serialize() here and avoid duplicating low-level serialization code?

Yes, it is possible. I'll do it.

dajac · 2016-01-19T19:03:12Z

@ewencp I have addressed your points. Now, version is explicitly handled and an exception is thrown if the version is not supported. I'm just wondering if throwing an exception is the best way especially when sendResponseCallback is called from the purgatory. Also, I have updated ProduceRequest.getErrorResponse to handle version 0 and 1.

…requests equivalents.

dajac · 2016-01-20T10:01:00Z

I have rebased and addressed all the comments.

ewencp · 2016-01-21T00:27:47Z

LGTM, thanks @dajac!

* Add the version field to JSON responses. * Add a default partition_load resource (disk) in case the user does not provide one. * Standardize servlet response format.

…Api (apache#110) TICKET = KAFKA-9253 LI_DESCRIPTION = There seems to be a race condition. When a ListPartitionReassignment request is processed, the partition reassignment has already been completed, and thus the ListPartitionReassignmentResponse does not contain data for that partition (tp2 in the test), which further causes a NullPointerException. EXIT_CRITERIA = When the fix for the flaky test is committed in upstream and picked up in this repo

Try to export all metrics available for faster troubleshooting.

Cl3 errors

dajac changed the title ~~KAFKA-2071: Replace Producer Request/Response with their org.apache.kafka.common.requests equivalents~~ KAFKA-2071 [WIP]: Replace Producer Request/Response with their org.apache.kafka.common.requests equivalents Aug 5, 2015

dajac force-pushed the KAFKA-2071 branch from 8e1ca3c to 4218822 Compare August 11, 2015 05:24

dajac changed the title ~~KAFKA-2071 [WIP]: Replace Producer Request/Response with their org.apache.kafka.common.requests equivalents~~ KAFKA-2071: Replace Producer Request/Response with their org.apache.kafka.common.requests equivalents Aug 11, 2015

dajac mentioned this pull request Aug 15, 2015

KAFKA-2072 [WIP]: Add StopReplica request/response to o.a.k.common.requests and replace the usage in core module #141

Closed

dajac force-pushed the KAFKA-2071 branch from 4218822 to 711ba33 Compare August 17, 2015 18:50

dajac force-pushed the KAFKA-2071 branch from 761a159 to a1dcbab Compare December 30, 2015 16:05

dajac force-pushed the KAFKA-2071 branch from a1dcbab to 133579d Compare January 7, 2016 18:14

granthenke reviewed Jan 7, 2016
View reviewed changes

ijuma reviewed Jan 8, 2016
View reviewed changes

Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated

ijuma Jan 8, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: You don't need () in sizeOf.

granthenke reviewed Jan 13, 2016
View reviewed changes

dajac force-pushed the KAFKA-2071 branch from 03513b4 to a9c0f86 Compare January 14, 2016 06:41

ewencp reviewed Jan 18, 2016
View reviewed changes

dajac added 3 commits January 20, 2016 07:24

Replace Producer Request/Response with their org.apache.kafka.common.…

945440d

…requests equivalents.

Address reviews.

f2f5aa8

Address reviews.

348f917

dajac force-pushed the KAFKA-2071 branch from 7ced7fb to 348f917 Compare January 20, 2016 06:25

Fix RequestResponseTest

a56a06c

asfgit closed this in 79cda04 Jan 21, 2016

dajac deleted the KAFKA-2071 branch August 11, 2020 06:46

davide-armand pushed a commit to aiven/kafka that referenced this pull request Dec 1, 2025

chore(tools): add "catch all" pattern for jmx metrics (apache#110)

1d8afae

Try to export all metrics available for faster troubleshooting.

fvaleri added a commit to fvaleri/kafka that referenced this pull request Mar 27, 2026

Merge pull request apache#110 from fvaleri/cl3-errors

1c22940

Cl3 errors

Uh oh!

Conversation

dajac commented Aug 4, 2015

Uh oh!

asfbot commented Aug 4, 2015

Uh oh!

asfbot commented Aug 11, 2015

Uh oh!

dajac commented Aug 15, 2015

Uh oh!

dajac commented Aug 17, 2015

Uh oh!

asfbot commented Aug 18, 2015

Uh oh!

dajac commented Aug 25, 2015

Uh oh!

asfbot commented Aug 25, 2015

Uh oh!

dajac commented Aug 25, 2015

Uh oh!

asfbot commented Aug 26, 2015

Uh oh!

dajac commented Aug 26, 2015

Uh oh!

granthenke commented Dec 11, 2015

Uh oh!

dajac commented Dec 18, 2015

Uh oh!

dajac commented Dec 30, 2015

Uh oh!

dajac commented Dec 30, 2015

Uh oh!

granthenke commented Jan 6, 2016

Uh oh!

ijuma commented Jan 6, 2016

Uh oh!

dajac commented Jan 7, 2016

Uh oh!

dajac commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

granthenke commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jan 8, 2016

Uh oh!

dajac commented Jan 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

granthenke commented Jan 13, 2016

Uh oh!

dajac commented Jan 14, 2016

Uh oh!

dajac commented Jan 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac commented Jan 19, 2016

Uh oh!

dajac commented Jan 20, 2016

Uh oh!

ewencp commented Jan 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects