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

Resolving TypeError, during version unpacking #2098

Merged
merged 8 commits into from
Aug 25, 2024

Conversation

couzhei
Copy link
Contributor

@couzhei couzhei commented Aug 14, 2024

This will make sure that the version unpacking doesn't stop connection to some versions of message brokers. For message brokers with normal-style version, say 3.13.6,

def version_string_as_tuple(s: str) -> version_info_t:
    """Convert version string to version info tuple."""
    v = _unpack_version(*s.split('.'))
    ...

will result in ['3', '13', '6'] list. But for newer releases of rabbitmq, say when you pull docker image using a specified version, in my case 4.0-rc as discussed in this issue related to #2088 it becomes this:

['4', '0', '0+beta', '2', '6', 'g65c3daf']

which causes that TypeError.

Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

Please add tests and fix the lint error.

@couzhei couzhei requested a review from Nusnus August 21, 2024 05:52
@couzhei
Copy link
Contributor Author

couzhei commented Aug 21, 2024

I really don't get the CI error. I even created a virtual environment from a 3.9 version, but it was okay there. I also have this test, almost always failed:

===================================================== short test summary info =====================================================
FAILED t/unit/transport/test_redis.py::test_Redis::test_publish__consume - OSError: [Errno 9] Bad file descriptor

Results (37.34s):
    1181 passed
       1 failed
         - t/unit/transport/test_redis.py:1373 test_Redis.test_publish__consume
     315 skipped
Restoring 2 unacknowledged message(s)

Which I'm not sure why it happens when I run tests locally.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

re run the job to see

@auvipy
Copy link
Member

auvipy commented Aug 21, 2024

we just need to fix the lint error

@couzhei
Copy link
Contributor Author

couzhei commented Aug 21, 2024

Thank you, my bad. I have never used pydocstyle, but I have just checked it out, and will be aware of it in the future.

@couzhei
Copy link
Contributor Author

couzhei commented Aug 21, 2024

re run the job to see

I think I don't have the right to re-run a job.

@couzhei couzhei requested a review from auvipy August 21, 2024 06:34
…n `_unpack_version` method

- `"` are sent back to `'` (apparently my company's formatter `black` is not compatible with `pydocstyle`, and mine integration with VSCode has messed with kombu's repository)
@thedrow thedrow merged commit f3ccc31 into celery:main Aug 25, 2024
16 checks passed
@couzhei couzhei deleted the bugfix/type-error-version-checking branch August 26, 2024 10:50
ivanprjcts added a commit to ivanprjcts/kombu that referenced this pull request Sep 14, 2024
@SandeepaDevin
Copy link

/python3.9/site-packages/kombu/transport/redis.py

GlobalKeyPrefixMixin | <class 'kombu.transport.redis.GlobalKeyPrefixMixin'>
InconsistencyError | <class 'kombu.exceptions.InconsistencyError'>
Mutex | <function Mutex at 0x7f1e4ee5eb80>
MutexHeld | <class 'kombu.transport.redis.MutexHeld'>
PRIORITY_STEPS | [0, 3, 6, 9]
READ | 1
VersionMismatch | <class 'kombu.exceptions.VersionMismatch'>
poll | <function poll at 0x7f1e4eeae3a0>
promise | <class 'vine.promises.promise'>
redis | None
register_after_fork | <function register_after_fork at 0x7f1e51c95f70>

I tried from supervisor and it seems like it cannot pick the redis version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants