KAFKA-9053: AssignmentInfo#encode hardcodes the LATEST_SUPPORTED_VERSION by ableegoldman · Pull Request #7537 · apache/kafka

ableegoldman · 2019-10-16T21:26:54Z

Also put in some additional logging that makes sense to add, and proved helpful in debugging this particular issue.

Unit tests verifying the encoded supported version were added.

This should get cherry-picked back to 2.1

ableegoldman · 2019-10-16T21:27:33Z

ableegoldman · 2019-10-16T21:39:27Z


        buf.putInt(usedVersion); // used version
-        buf.putInt(LATEST_SUPPORTED_VERSION); // supported version
+        buf.putInt(latestSupportedVersion); // supported version


This is actually not necessary for SubscriptionInfo at this point in time, but may prevent a similar bug in the future.

Sounds good. Hopefully we do not need to decode and encode a subscription info object within the same instance :P

HahaI guess you never know

ableegoldman · 2019-10-16T21:40:03Z

    }

+    @Test
+    public void shouldEncodeAndDecodeVersion5() {


Just a missing test I noticed

bbejeck

Thanks for the fix @ableegoldman LGTM

bbejeck · 2019-10-16T22:40:46Z

FYI with this fix all tests passed on a branch of this system test #7529

http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2019-10-16--001.1571262880--ableegoldman--KIP-429-sysTest-logging--b2e7d45/report.html

guozhangwang · 2019-10-16T22:41:15Z

                    "Sent a version {} subscription and got version {} assignment back (successful version probing). "
                        +
-                        "Downgrade subscription metadata to commonly supported version and trigger new rebalance.",
+                        "Downgrade subscription metadata to commonly supported version {} and trigger new rebalance.",


nitpick: let's just use downgrade than downgrading in the log4j?

Ack, will replace all with downgrade

guozhangwang · 2019-10-16T22:43:40Z


        buf.putInt(usedVersion); // used version
-        buf.putInt(LATEST_SUPPORTED_VERSION); // supported version
+        buf.putInt(latestSupportedVersion); // supported version


Sounds good. Hopefully we do not need to decode and encode a subscription info object within the same instance :P

guozhangwang · 2019-10-16T23:13:00Z

LGTM, will merge after jenkins build.

ableegoldman · 2019-10-17T04:02:01Z

@guozhangwang Java 11/2.13 passed, Java 8 failed with unrelated flaky
kafka.api.SslAdminClientIntegrationTest.testCreateTopicsResponseMetadataAndConfig
kafka.network.DynamicConnectionQuotaTest.testDynamicConnectionQuota

Java 11/2.12 failed with unknown/unnamed core integration test
Execution failed for task ':core:integrationTest'

…ION (#7537) Also put in some additional logging that makes sense to add, and proved helpful in debugging this particular issue. Unit tests verifying the encoded supported version were added. This should get cherry-picked back to 2.1 Reviewers: Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

guozhangwang · 2019-10-17T05:25:33Z

@ableegoldman I've merged to trunk and 2.4, but it took me quite some time trying to resolve conflicts for 2.3 and older versions and I'm not very confident my resolution would not introduce any regressions. Could you create a separate PR for 2.3-2.1 (one PR should be fine since there's not too much changes)?

Same as #7537 but targeted at 2.3 for cherry-pick Reviewers: Bill Bejeck <bbejeck@gmail.com>

* KAFKA-8649: send latest commonly supported version in assignment (apache#7425) Reviewers: Matthias J. Sax <matthias@confluent.io> * KAFKA-8262, KAFKA-8263: Fix flaky test `MetricsIntegrationTest` (apache#6922) (apache#7457) - Timeout occurred due to initial slow rebalancing. - Added code to wait until `KafkaStreams` instance is in state RUNNING to check registration of metrics and in state NOT_RUNNING to check deregistration of metrics. - I removed all other wait conditions, because they are not needed if `KafkaStreams` instance is in the right state. Reviewers: Guozhang Wang <wangguoz@gmail.com> * MINOR: clarify wording around fault-tolerant state stores (apache#7510) Reviewers: Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io> * MINOR: Port changes from trunk for test stability to 2.3 branch (apache#7424) Reviewer: Matthias J. Sax <matthias@confluent.io> * KAFKA-9014: Fix AssertionError when SourceTask.poll returns an empty list (apache#7491) Author: Konstantine Karantasis <konstantine@confluent.io> Reviewer: Randall Hauch <rhauch@gmail.com> * KAFKA-8945/KAFKA-8947: Fix bugs in Connect REST extension API (apache#7392) Fix bug in Connect REST extension API caused by invalid constructor parameter validation, and update integration test to play nicely with Jenkins Fix instantiation of TaskState objects by Connect framework. Author: Chris Egerton <chrise@confluent.io> Reviewers: Magesh Nandakumar <mageshn@confluent.io>, Randall Hauch <rhauch@gmail.com> * KAFKA-8340, KAFKA-8819: Use PluginClassLoader while statically initializing plugins (apache#7315) Added plugin isolation unit tests for various scenarios, with a `TestPlugins` class that compiles and builds multiple test plugins without them being on the classpath and verifies that the Plugins and DelegatingClassLoader behave properly. These initially failed for several cases, but now pass since the issues have been fixed. KAFKA-8340 and KAFKA-8819 are closely related, and this fix corrects the problems reported in both issues. Author: Greg Harris <gregh@confluent.io> Reviewers: Chris Egerton <chrise@confluent.io>, Magesh Nandakumar <mageshn@confluent.io>, Konstantine Karantasis <konstantine@confluent.io>, Randall Hauch <rhauch@gmail.com> * MINOR: log reason for fatal error in locking state dir (apache#7534) Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io> * KAKFA-8950: Fix KafkaConsumer Fetcher breaking on concurrent disconnect (apache#7511) The KafkaConsumer Fetcher can sometimes get into an invalid state where it believes that there are ongoing fetch requests, but in fact there are none. This may be caused by the heartbeat thread concurrently handling a disconnection event just after the fetcher thread submits a request which would cause the Fetcher to enter an invalid state where it believes it has ongoing requests to the disconnected node but in fact it does not. This is due to a thread safety issue in the Fetcher where it was possible for the ordering of the modifications to the nodesWithPendingFetchRequests to be incorrect - the Fetcher was adding it after the listener had already been invoked, which would mean that pending node never gets removed again. This PR addresses that thread safety issue by ensuring that the pending node is added to the nodesWithPendingFetchRequests before the listener is added to the future, ensuring the finally block is called after the node is added. Reviewers: Tom Lee, Jason Gustafson <jason@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com> * Fix bug in AssignmentInfo#encode and add additional logging (apache#7545) Same as apache#7537 but targeted at 2.3 for cherry-pick Reviewers: Bill Bejeck <bbejeck@gmail.com> * Bump version to 2.3.1 * Update versions to 2.3.2-SNAPSHOT * HOTFIX: fix bug in VP test where it greps for the wrong log message (apache#7643) Reviewers: Guozhang Wang <wangguoz@gmail.com>

…TED_VERSION (apache#7537)" This reverts commit 78f5da9.

Fix bug in AssignmentInfo#encode and add additional logging

7d8951e

ableegoldman changed the title ~~KAFKA-9053: AssignmentInfo#encode hardcodes the LATEST_SUPPORTED_VERSION~~ KAFKA-9053: AssignmentInfo#encode hardcodes the LATEST_SUPPORTED_VERSION Oct 16, 2019

ableegoldman changed the title ~~KAFKA-9053: AssignmentInfo#encode hardcodes the LATEST_SUPPORTED_VERSION~~ KAFKA-9053: AssignmentInfo#encode hardcodes the LATEST_SUPPORTED_VERSION Oct 16, 2019

add tests

9c73fab

ableegoldman commented Oct 16, 2019

View reviewed changes

mjsax added the streams label Oct 16, 2019

bbejeck approved these changes Oct 16, 2019

View reviewed changes

guozhangwang approved these changes Oct 16, 2019

View reviewed changes

downgrade

a43f088

guozhangwang merged commit 78f5da9 into apache:trunk Oct 17, 2019

ableegoldman mentioned this pull request Oct 17, 2019

KAFKA-9053: AssignmentInfo#encode hardcodes the LATEST_SUPPORTED_VERSION #7545

Merged

bbejeck pushed a commit that referenced this pull request Oct 17, 2019

Fix bug in AssignmentInfo#encode and add additional logging (#7545)

a0ddc05

Same as #7537 but targeted at 2.3 for cherry-pick Reviewers: Bill Bejeck <bbejeck@gmail.com>

bbejeck pushed a commit that referenced this pull request Oct 17, 2019

Fix bug in AssignmentInfo#encode and add additional logging (#7545)

75d1c0b

Same as #7537 but targeted at 2.3 for cherry-pick Reviewers: Bill Bejeck <bbejeck@gmail.com>

bbejeck pushed a commit that referenced this pull request Oct 17, 2019

Fix bug in AssignmentInfo#encode and add additional logging (#7545)

16e3c52

Same as #7537 but targeted at 2.3 for cherry-pick Reviewers: Bill Bejeck <bbejeck@gmail.com>

bbejeck mentioned this pull request Oct 23, 2019

MINOR: Streams upgrade system test cleanup #7571

Merged

3 tasks

ableegoldman added a commit to ableegoldman/kafka that referenced this pull request Nov 13, 2019

Revert "KAFKA-9053: AssignmentInfo#encode hardcodes the LATEST_SUPPOR…

53961d4

…TED_VERSION (apache#7537)" This reverts commit 78f5da9.

Conversation

ableegoldman commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ableegoldman commented Oct 16, 2019

Uh oh!

ableegoldman Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

guozhangwang Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ableegoldman Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

ableegoldman Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Oct 16, 2019

Uh oh!

guozhangwang Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ableegoldman Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

guozhangwang Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Oct 16, 2019

Uh oh!

ableegoldman commented Oct 17, 2019

Uh oh!

guozhangwang commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ableegoldman commented Oct 16, 2019 •

edited

Loading