Skip to content

Conversation

@Advaitgaur004
Copy link
Contributor

What is the purpose of the change

  1. Removes redundant status check in SingleMessageDeliverer.get_nowait()
  2. Corrects dictionary key checking in cluster directories

Brief changelog

  • src/dubbo/deliverers.py:

    • Removed redundant status check in get_nowait() method
    • Improved state transition logic
  • src/dubbo/cluster/directories.py:

    • Fixed incorrect dictionary key checking from .items() to direct key lookup

Verifying this change

  1. Manual Testing
  • Tested message delivery with various status combinations
  • Verified directory updates with multiple URLs

Checklist

  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • GitHub Actions also works fine on my and this branch.

# create new invokers
for url in urls:
k = str(url)
if k in old_invokers.items():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorrect as items() return a list of tuples.

return self._message

# raise error
if self._status is DelivererStatus.FINISHED:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant, as it never entered this block if the previous condition is true; therefore, this block statement is impossible.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, but the entire delivers module has been deprecated and is no longer referenced by any other module. Could you please remove the entire module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay i'll do that

@Advaitgaur004
Copy link
Contributor Author

@AlbumenJ @cnzakii Have a look.

… furthermore, the documentation does not mention this module
Copy link
Member

@cnzakii cnzakii left a comment

Choose a reason for hiding this comment

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

LGTM

@Advaitgaur004
Copy link
Contributor Author

find . -type f -name "test_*.py" -exec grep -l "deliverers" {} \; gives nothing as not test refer this module.

  • No other module imports or uses it
  • Its functionality (message delivery) is handled by other mechanisms
  • No tests reference it
  • No configuration uses it
  • Not mentioned in examples or documentation

#42 Updated

Safely Removed deliverers module

@AlbumenJ AlbumenJ merged commit 2210d84 into apache:main Jan 21, 2025
@Advaitgaur004 Advaitgaur004 changed the title Correct logic in message delivery and directory management deliverers module is deprecated and removed gracefully from the system Jan 21, 2025
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.

3 participants