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

Fixes the data type of the expected_size variable #2438

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hiwakaba
Copy link
Contributor

@hiwakaba hiwakaba commented Jun 17, 2024

Hello, this patch changes the data type of the expected_size variable from float to integer.

The expected_size variable could currently be a float when builder.compression_rate() returns a float value, but I think it should be an integer because expected_size represents the size of a MemoryRecordsBuilder object in bytes.

P.S. I got a test_memory_records_builder failure on Python-3.13.b02 in Fedora Linux.
https://bugzilla.redhat.com/show_bug.cgi?id=2290344

Actual Results:  
=================================== FAILURES ===================================
_______________________ test_memory_records_builder[1-2] _______________________
magic = 1, compression_type = 2
    @pytest.mark.parametrize("compression_type", [0, 1, 2, 3])
    @pytest.mark.parametrize("magic", [0, 1, 2])
    def test_memory_records_builder(magic, compression_type):
        builder = MemoryRecordsBuilder(
            magic=magic, compression_type=compression_type, batch_size=1024 * 10)
        base_size = builder.size_in_bytes()  # V2 has a header before
    
        msg_sizes = []
        for offset in range(10):
            metadata = builder.append(
                timestamp=10000 + offset, key=b"test", value=b"Super")
            msg_sizes.append(metadata.size)
            assert metadata.offset == offset
            if magic > 0:
                assert metadata.timestamp == 10000 + offset
            else:
                assert metadata.timestamp == -1
            assert builder.next_offset() == offset + 1
    
        # Error appends should not leave junk behind, like null bytes or something
        with pytest.raises(TypeError):
            builder.append(
                timestamp=None, key="test", value="Super")  # Not bytes, but str
    
        assert not builder.is_full()
        size_before_close = builder.size_in_bytes()
        assert size_before_close == sum(msg_sizes) + base_size
    
        # Size should remain the same after closing. No trailing bytes
        builder.close()
        assert builder.compression_rate() > 0
        expected_size = size_before_close * builder.compression_rate()
        assert builder.is_full()
>       assert builder.size_in_bytes() == expected_size
E       assert 241 == 241.00000000000003
E        +  where 241 = <bound method MemoryRecordsBuilder.size_in_bytes of <kafka.record.memory_records.MemoryRecordsBuilder object at 0x7f597eba2070>>()
E        +    where <bound method MemoryRecordsBuilder.size_in_bytes of <kafka.record.memory_records.MemoryRecordsBuilder object at 0x7f597eba2070>> = <kafka.record.memory_records.MemoryRecordsBuilder object at 0x7f597eba2070>.size_in_bytes
test/record/test_records.py:203: AssertionError
=========================== short test summary info ============================
FAILED test/record/test_records.py::test_memory_records_builder[1-2] - assert...
================= 1 failed, 1081 passed, 33 skipped in 10.95s ==================

Thanks in advance,
Hirotaka


This change is Reviewable

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.

None yet

1 participant