Skip to content
This repository was archived by the owner on Jul 15, 2023. It is now read-only.

Gfodor/add timestamp to producer #44

Closed
wants to merge 36 commits into from
Closed

Gfodor/add timestamp to producer #44

wants to merge 36 commits into from

Conversation

gfodor
Copy link

@gfodor gfodor commented Apr 3, 2016

This PR updates my previous PR #31 --

  • Updates KafkaNet to support FetchRequest message version 1, which includes a ThrottleTime (see comments in my previous PR. Also as noted on that thread, the CurrentVersion of FetchRequest is retained at 0, and callers will need to explicitly request version 1 requests to be made in order to include ThrottleTime.
  • Updates KafkaNet to support V1 message format and up to V2 ProducerRequest/Response format from KIP-32 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-32+-+Add+timestamps+to+Kafka+message) This introduces the ability for producers to specify a timestamp, and consumers to read timestamps from messages. This new message format is due to be available with Kafka 0.10.0.

@msftclas
Copy link

msftclas commented Apr 3, 2016

Hi @gfodor, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@@ -307,26 +410,30 @@ internal static Message ParseFrom(KafkaBinaryReader reader, long offset, int siz
/// <summary>
/// Clean up attributes for message, otherwise there is double decompress at kafka broker side.
/// </summary>
internal void CleanMagicAndAttributesBeforeCompress()
internal void CleanAttributesBeforeCompress()
{
this.Attributes = 0;
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't completely sure about this change. It looks like based on the comments that someone was experiencing a magic value of 2, and hence was zeroing things out here, but magic value of 2 is an invalid value. It's now important that the packed messages retain their magic value of either 0 or 1 otherwise the timestamp will not be getting encoded properly.

@jthelin
Copy link
Contributor

jthelin commented Apr 4, 2016

It seems like there are at least two orthogonal[?] things contained in this Pull Request - support for different protocol versions, and the timestamp change itself.

Could these be split into separate Pull Requests?

It is very much easier for others to assess the contents and impact of a change(s) that way.

Also, @gfodor don't be afraid to rewrite / rebase your PR commits to clean up the change history -- you can do interactive rebase on your local branch and then force push to the remote PR branch and GitHub will do the right thing and keep everything associated with this PR discussion thread.
#11 (comment)

@gfodor
Copy link
Author

gfodor commented Apr 4, 2016

I think the way to do it would be 0.9.0 support and then 0.10.0 support. The protocol changes for 0.9.0 are adding the throttle time fields (FetchRequest message format v1, ProducerResponse format v1), and 0.10.0 is adding timestamp fields support (Message format v1, ProducerResponse format v2.) I can pull out the 0.9.0 stuff separately if that would be useful.

gitter-badger and others added 23 commits April 28, 2016 09:00
consumedOffset=fetchOffset which could lead to data loss. Don't commit
offset if it already commited
	modified:   src/KafkaNET.Library/Consumers/ZookeeperConsumerConnector.cs
	modified:   src/KafkaNET.Library/ZooKeeperIntegration/Listeners/ZKRebalancerListener.cs
Pretty self explanatory
- NuGet package cache will be restored by Visual Studio during build.
- Visual Studio will automatically create these files during build when needed.
- The Reference paths in project file `Kafka.Client.Tests.csproj` and NuGet `packages.config` file in that project specify different versions of the `FluentAssertions` and `Moq` packages.

- Fix is to change `packages.config` file to specify same version as the `.csproj` file.
- The NuGet `packages.config` files are missing from projects `KafkaNET.Library` and `KafkaNETLibraryConsole`
- Adding some missing package dependencies that Visual Studio 2013 complained were missing.

- Ran the migrateToAutomaticPackageRestore.ps1 script to fix problems with nuget package auto-restore when building from inside Visual Studio.

  https://github.com/owen2/AutomaticPackageRestoreMigrationScript
This reverts commit 84b459b.
…ed to itself

The kind people at Viva64 gave an evaluation license for PVS-Studio to try out on some OSS projects, and running it on Kafkanet source code found this problem.
http://www.viva64.com/en/pvs-studio/

Error V3005. The 'ConsumeGroupFindNewLeaderSleepIntervalMs' variable is assigned to itself
http://www.viva64.com/en/d/0403/print/

Fixes issue #20
@gfodor
Copy link
Author

gfodor commented Apr 28, 2016

This branch got hosed when trying to do my rebasing, going to just close it and create another one.

@gfodor gfodor closed this Apr 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.