Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes the data type of the expected_size variable #2438

Merged
merged 1 commit into from
Feb 7, 2025

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

@dpkp dpkp merged commit 47c1be6 into dpkp:master Feb 7, 2025
4 of 21 checks passed
@dpkp
Copy link
Owner

dpkp commented Feb 7, 2025

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants