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 github.com/harlow/kinesis-consumer from v0.3.6-0.20240606153816-553e2392fdf3 to v0.3.6-0.20240916192723-43900507c911 #15890

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

mskonovalov
Copy link
Contributor

@mskonovalov mskonovalov commented Sep 16, 2024

Summary

OK, fixing this #13853
Specifically #13853 (comment)

There are a few issues:

  1. the library kinesis-consumer seem to be abandoned :(
  2. recently (and it coincided with the switch to AWS SDK v2) 2 bugs were introduced into the library: ProvisionedThroughputExceededException Error harlow/kinesis-consumer#158 and Retryable errors don't get retried in master harlow/kinesis-consumer#152
    Here are the PRs fix isRetriableError harlow/kinesis-consumer#159 (from Jun 12 2024, not reviewed, not accepted) and Fix ProvisionedThroughputExceededException error harlow/kinesis-consumer#161 (from 16 Sept 2024)

Solution:
Unfortunately, I don't know how to handle the case with unmaintained library.
For now I made a personal fork, created a tag with 2 bug fixes and switched Telegraf to use this fork.
Not sure if it is a good idea in the long term, I don't think I will be able to maintain the fork properly (and not sure if I have an access to real Kinesis in the future, currently testing it against localstack)

There are no good alternatives to this library as far as I can tell.
Official AWS KClv2 is only in Java or Python https://docs.aws.amazon.com/streams/latest/dev/developing-consumers-with-kcl-v2.html
One potentially good option could be KCL lib from VMWare:
they had a first version https://github.com/vmware/vmware-go-kcl but only for AWS SDK v1.
Then they created a new one for AWS SDK v2 https://github.com/vmware/vmware-go-kcl-v2
but it only supports go 1.21 and last commit 6 months ago :( also no activity on PRs for a while.

So please let me know what is the preferred way forward

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #13853

@telegraf-tiger telegraf-tiger bot added the chore label Sep 16, 2024
@srebhan srebhan self-assigned this Sep 16, 2024
@srebhan
Copy link
Member

srebhan commented Sep 16, 2024

@mskonovalov switching an upstream library to one-man forks has proven bad in the past especially since the likelihood of you lose interest in maintaining the fork is high. This being said, there has to be a REALLY strong justification for doing such a change...

As I'm not too familiar with AWS kinesis, could you please answer the following questions regarding your reasonging:

  1. What makes you think the upstream library is not maintained anymore? Could you point me to a clear statement of the maintainer?
  2. If switching libraries is unavoidable, why not using the upstream SDK provided by Amazone?

Looking forward to hear your thoughts and reasons!

@mskonovalov
Copy link
Contributor Author

mskonovalov commented Sep 16, 2024

Hi @srebhan
I absolutely agree, but ATM Telegraf is unusable with kinesis input plugin because of those bugs. Telegraf users has to roll back to the old version of Telegraf which is much worse solution that switching to a one-man fork :)
As I mentioned above - it is a short term solution to fix the bug while discussing what would be the long-term approach for that.

I'll start in the opposite order:
2. AWS SDK is too low level - there is no SDK for consumer scenario that would await for incoming messages. It allows you to only get available messages once. So if you really want to have kinesis-consumer you need to implement it yourself (inside Telegraf) or use any existing library. To understand better above I posted a link to the official AWS implementation of the kinesis-consumer but only for Java and Python platforms https://docs.aws.amazon.com/streams/latest/dev/developing-consumers-with-kcl-v2.html
Ideally what we need here is the similar official KCL lib from AWS for Golang.

  1. There is no official message that the library is not maintained. What makes me think it is likely the case are the following facts:
    a) last release of the lib was on Sept 2021 - 3 years ago https://github.com/harlow/kinesis-consumer/releases/tag/v0.3.5
    b) Telegraf is using latest commit from the master branch instead of official relesed version
    c) issue related to isRetriable Retryable errors don't get retried in master harlow/kinesis-consumer#152 was reported back in Mar 2023 - more than 1 year ago, PR with the fix for that issue fix isRetriableError harlow/kinesis-consumer#159 has been submitted 4 months ago and still not reviewed or merged
    d) the PR that introduced the 2nd bug Maintain parent/child shard ordering across shard splits/merges. harlow/kinesis-consumer#155 has been created in Aug 2023 (1 year ago) and then merged into master on Jun 2024 (10 months later) without a single comment or proper testing :(

As I said in the PR description - I'm happy to discuss the best way to move forward. I made fixes for these bugs but I cannot push them to the original repository. It may get accepted and merged soon or maybe never, who knows. I just would like to fix the issue for everyone who cannot use kinesis consumer in Telegraf ATM.
Thanks

@mskonovalov
Copy link
Contributor Author

I'd say another alternative would be to pull the kinesis-consumer code into Telegraf :D

@harlow
Copy link

harlow commented Sep 16, 2024

Hi @mskonovalov sorry for the pains this has caused you. i'd be happy to transfer this codebase or give you admin rights to the repo so you can make changes as needed

@mskonovalov
Copy link
Contributor Author

Hey @harlow thanks for accepting the PRs, I'll change this one to point to a most recent commit.
TBH sharing admin permissions with someone else might be not a stupid idea :)
Though, some long term decision need to be made regarding how to maintain the library. Not sure if you know some of the big consumers of the library, so maybe somebody from Telergaf maintainers (as potentially one of the big consumers) might be interested in having admin permissions as well :)

@mskonovalov mskonovalov changed the title chore: switch kinesis-consumer to a fork chore: bump kinesis-consumer version Sep 17, 2024
@srebhan
Copy link
Member

srebhan commented Sep 18, 2024

So this is the solution I've hoped for! Thanks @mskonovalov for digging into this and thanks @harlow for giving away part of your baby, I know how hard this is. ;-)

@srebhan srebhan changed the title chore: bump kinesis-consumer version chore(deps): Bump github.com/harlow/kinesis-consumer from v0.3.6-0.20240606153816-553e2392fdf3 to v0.3.6-0.20240916192723-43900507c911 Sep 18, 2024
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks @mskonovalov!

@srebhan srebhan added dependencies Pull requests that update a dependency file ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Sep 18, 2024
@srebhan srebhan assigned DStrand1 and unassigned srebhan Sep 18, 2024
@DStrand1 DStrand1 merged commit 024b2e2 into influxdata:master Sep 19, 2024
29 checks passed
@github-actions github-actions bot added this to the v1.32.1 milestone Sep 19, 2024
@mskonovalov mskonovalov deleted the kinesis-consumer-rate-limit branch September 20, 2024 03:35
srebhan pushed a commit that referenced this pull request Oct 7, 2024
…240606153816-553e2392fdf3 to v0.3.6-0.20240916192723-43900507c911 (#15890)

(cherry picked from commit 024b2e2)
asaharn pushed a commit to asaharn/telegraf that referenced this pull request Oct 16, 2024
…240606153816-553e2392fdf3 to v0.3.6-0.20240916192723-43900507c911 (influxdata#15890)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore dependencies Pull requests that update a dependency file ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kinesis consumer not working from 1.20.3 onwards
4 participants