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

Housekeeping #9

Merged
merged 3 commits into from
Jan 11, 2023
Merged

Housekeeping #9

merged 3 commits into from
Jan 11, 2023

Conversation

dnwe
Copy link
Collaborator

@dnwe dnwe commented Jan 10, 2023

  • chore: add go.mod and go.sum
  • fix: migrate go-fuzz to native Go 1.18 test
  • chore: apply gofmt

- convert corpus using file2fuzz from golang.org/x/tools
- move fuzz.go to fuzz_test.go and use testing.F
- run with `go test -parallel=1 -fuzz FuzzData`

func FuzzData(f *testing.F) {
f.Fuzz(func(t *testing.T, data []byte) {
func() int {
Copy link
Owner

Choose a reason for hiding this comment

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

🤔 I'm not sure what the inner nested anonymous func is for? I'm not that familiar with the new f.Fuzz though...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eapache yeah that's the fuzz target (as opposed to the fuzz test) which defines what arguments to expect from the corpus and testcases:

image

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah sorry that part makes sense, I was referring to the third inner func() int { }() which doesn’t have an apparent purpose?

@eapache
Copy link
Owner

eapache commented Jan 11, 2023

I'll be honest, I'd kind of forgotten this repo existed, and I hadn't looked at Sarama in a few years either. So, wow, this PR turned into a real trip down memory lane. But thank you for the much-needed cleanup!

And since I'm already off-topic for the actual PR, let me add: thank you for your work on Sarama! It is absolutely wild to me that it is still being heavily used and maintained, by people who are not me, a full decade after I wrote the first draft. It feels good :)


To my surprise, I see this is still in active use by Sarama (why has upstream Kafka still not fixed their snappy streaming format? sigh). This package is pretty trivial and I'm happy to keep merging housekeeping PRs every couple of years, but that seems like a bad plan for long-term maintainability, as I no longer work at Shopify or have any regular interaction with Sarama (or really Kafka, or golang, for that matter).

I'm not sure if there's a better option (moving it back into Sarama feels wrong... I can imagine other projects needing this library, though I don't actually know of any, 5 years later). But just so you're aware, since it looks like you're quite active on Sarama repo right now.

@eapache eapache merged commit bf00bc1 into eapache:master Jan 11, 2023
@dnwe dnwe deleted the housekeeping branch January 11, 2023 11:41
@dnwe
Copy link
Collaborator Author

dnwe commented Jan 11, 2023

@eapache 👋 thanks, yeah eapache/queue is still in-use by Sarama too :)

If you want to add me as a contributor and/or move the repo to some generic org for future maintainership then that'd be fine. Alternatively I could just pull snappy.go itself into Shopify/sarama if you want to freeze this repo as archived

@eapache
Copy link
Owner

eapache commented Jan 11, 2023

yeah eapache/queue is still in-use by Sarama too :)

Oof, yeah. Speaking of housekeeping that should really be updated to use generics. (Actually, now that generics are a thing, there must surely be a better, general data structures package for go which provides an efficient queue implementation?). Anyway.

In the short term I will make you a contributor to both repos. If at some point the Sarama team (is that even a shopify-led project anymore?) want to fork or take full ownership or something, just give me a ping and I’ll be happy to have that conversation.

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.

2 participants