Skip to content

Conversation

@vrischmann
Copy link
Contributor

Hi,

I've started to work on adding zstd support but it turns out it's vastly more complicated than the previous compression codecs since it requires implementing several things:

I've started implementing some stuff but before going further I have some questions:

  • there's no way in Conn.ReadBatch for me to know if I should make a v10 request because the user wants zstd or not as far as I can see because the ConnConfig does not have any compression configuration. Should I just had it there or do you have another idea ?
  • since the v10 fetch request is going to be useful for more than just zstd (transactions, fetch sessions, and some other stuff were added since v2) maybe it's better to do that in a more generic way.
  • I can access the codec in Conn.WriteCompressedMessages so here I could switch request and response version based on it but again, is there a better alternative ?

Anyway, I originally thought this would be an easy thing to add so I can get accustomed to the codebase, clearly I was mistaken :)

@VictorDenisov
Copy link
Contributor

@vrischmann Part of what you are doing is being implemented here: #146, #160, #163 Maybe we can combine our efforts so that we don't do work twice.

@vrischmann
Copy link
Contributor Author

Thanks for pointing me to that, I'll take a look. I agree there's no point in duplicating work.

@Pryz Pryz self-assigned this Feb 1, 2019
@Pryz
Copy link
Contributor

Pryz commented Mar 15, 2019

Hey @vrischmann, Records and RecordBatch are now supported in kafka-go. Do you want to keep working on ztsd support ?

@vrischmann
Copy link
Contributor Author

Hello. It's been a while since I've touched this code and I don't have immediate plans to work on it no, so if anyone else wants to tackle zstd feel free.

@achille-roussel
Copy link
Contributor

@Pryz is going to clean this up and repackage in a different PR, thanks for your help on this @vrischmann !

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.

4 participants