Skip to content

Conversation

@tvoinarovskyi
Copy link
Collaborator

Tried to fix #1270 and found, that it did not work properly in the first place. Was a bug in FutureRecordMetadata, that ignored timestamp=-1 being returned from the broker.

Also minor fixes include:

  • Avoid getting current timestamp if magic==0 (broker <= 0.9), as it's not needed anyway. Will return -1 for this case in RecordMetadata.
  • Fixed timestamp error message if it's of improper type.
  • Added more tests for LegacyBatch
  • Added integration test for correct RecordMetadata fields, including checksum

Copy link
Owner

@dpkp dpkp left a comment

Choose a reason for hiding this comment

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

lgtm!

retries=5,
max_block_ms=10000,
compression_type=compression,
value_serializer=str.encode)
Copy link
Owner

Choose a reason for hiding this comment

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

You're sending bytes below so no serializer needed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy-paste)

return cls.LOG_OVERHEAD + cls.record_size(magic, key, value)


class LegacyRecordMetadata(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I am clearly missing something... What is the purpose of this class? It doesn't seem to do anything other than enforcing read-only on the attributes. Is there something else it does that I'm not realizing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's more or less as you said. It's just a class to store results of a function. I find this a bit more clean than namedtuple when it comes to optimization. This is a hot part.

It's a more convenient way of returning several values from a function. Returning a tuple will require you to parse it every time, but returning either None or an instance will only require a None check. See https://github.com/dpkp/kafka-python/pull/1273/files#diff-261f56705197d1ab9e6fca9249856556 and https://github.com/dpkp/kafka-python/pull/1273/files#diff-d98d7e54a13e15511c44d90e78f105e9R368.

@tvoinarovskyi tvoinarovskyi merged commit a345dcd into master Oct 22, 2017
@tvoinarovskyi tvoinarovskyi deleted the fix_timestamp_on_None branch October 22, 2017 07:56
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.

Degrade after LegacyRecords merge

4 participants