Skip to content
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

chore(deps): bump sarama to v1.41.0 #3108

Merged
merged 2 commits into from
Aug 30, 2023
Merged

chore(deps): bump sarama to v1.41.0 #3108

merged 2 commits into from
Aug 30, 2023

Conversation

dnwe
Copy link
Contributor

@dnwe dnwe commented Aug 29, 2023

Description

Bump sarama to v1.41.0

Also remove old module replace pin to v1.37.2 as that shouldn't be necessary since dapr/docs#3474 added documentation to add a config version pin for Azure EventHubs users instead.

Note: the module path has changed to github.com/IBM/sarama since ownership transitioned away from Shopify

Issue reference

Fixes #3104

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

@dnwe dnwe requested review from a team as code owners August 29, 2023 16:50
Also remove old module replace pin to v1.37.2 as that shouldn't be
necessary since dapr/docs#3474 added
documentation to add a config version pin for Azure EventHubs users
instead.

Note: the module path has changed to github.com/IBM/sarama since
ownership transitioned away from Shopify

Fixes dapr#3104

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
@berndverst
Copy link
Member

berndverst commented Aug 29, 2023

Thanks for the PR. Unfortunately I cannot accept this PR. Please do not upgrade any further deps at this time. I upgraded everything there was to upgrade safely yesterday and merged that this morning. We cannot safely upgrade sarama at this time.

See #3106 for a summary.

@berndverst berndverst closed this Aug 29, 2023
@berndverst
Copy link
Member

berndverst commented Aug 29, 2023

The original issue with sarama were these:
#2755
#2874

After upgrading sarama we encountered several issues that required a hotfix release. For this reason we will not take the risk to upgrade sarama at this time (just ahead of the 1.12 release).

To upgrade this library we will need to perform further manual testing and we do not have the bandwidth at this time.

@berndverst
Copy link
Member

@dnwe I'll see if we can run some additional tests - if we can get EventHubs to work with this version we can reopen this PR. That being said, I'm not sure anyone has time ahead of the 1.12 release unfortunately.

@dnwe
Copy link
Contributor Author

dnwe commented Aug 29, 2023

@berndverst yes no problem, I expect IBM/sarama#2470 was the underlying issue

The problem with supporting Azure Event Hubs is that it isn't Apache Kafka under the covers, it's an intermediate proxy that supports a subset of the Kafka APIs at various versions and then maps them onto Event Hubs protocol(s) at the backend. As as result Sarama's current mechanism of specifying the KAFKA_VERSION to determine what protocol versions to support and use doesn't really work properly with Event Hubs because it supports an unusual set of protocols and even defines minimum versions for ProduceRequest (v3) and FetchRequest (v4). For some reason EventHubs is very behind on FetchRequest and MetadataRequest so the max configuration you can use in Sarama is V1_0_0_0.

Here are the equivalent max versions for Kafka 1.0 and EventHubs:

    APIs                                 1.0  EventHubs
0   ProduceRequest                       v5   v7
1   FetchRequest                         v6   v6
2   ListOffsetsRequest                   v2   v7
3   MetadataRequest                      v5   v5
8   OffsetCommitRequest                  v3   v8
9   OffsetFetchRequest                   v3   v8
10  FindCoordinatorRequest               v1   v1
11  JoinGroupRequest                     v2   v7
12  HeartbeatRequest                     v1   v4
13  LeaveGroupRequest                    v1   v4
14  SyncGroupRequest                     v1   v5
15  DescribeGroupsRequest                v1   v5
16  ListGroupsRequest                    v1   v4
17  SaslHandshakeRequest                 v1   v1
18  ApiVersionsRequest                   v1   v3
19  CreateTopicsRequest                  v2   v6
20  DeleteTopicsRequest                  v1   v2
22  InitProducerIdRequest                v0   v1
23  OffsetForLeaderEpochRequest          v0   v0
32  DescribeConfigsRequest               v0   v2
36  SaslAuthenticateRequest              v0   v1
37  CreatePartitionsRequest              v0   v1
42  DeleteGroupsRequest                       v0
47  OffsetDeleteRequest                       v0

@berndverst
Copy link
Member

berndverst commented Aug 30, 2023

@dnwe That issue was actually filed by us :D (@DeepanshuA is is an approver of this repo).

But I think after much investigation we too determined that customers must set the Kafka version to V1, we even updated the Dapr documentation to call this out.

So perhaps with that in mind we can try upgrading the sarama version. The more I read about the issue you are trying to fix by upgrading Sarama, I think I'd rather break EventHubs in the worst case :)

@berndverst berndverst reopened this Aug 30, 2023
@berndverst berndverst added this to the v1.12 milestone Aug 30, 2023
Signed-off-by: Bernd Verst <github@bernd.dev>
@berndverst berndverst added this pull request to the merge queue Aug 30, 2023
Merged via the queue into dapr:master with commit 1c657f7 Aug 30, 2023
83 of 85 checks passed
@dnwe
Copy link
Contributor Author

dnwe commented Aug 30, 2023

@berndverst ah thanks, I didn't realise you were going to re-open and merge this.

I followed up with #3109 to put an additional safety blanket in-place that should prevent you from getting issues raised by Event Hubs users

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Kafka sarama package to 1.41
2 participants