Skip to content

enhancement: implement multiton ListenerManager #1094

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.

Sign up for GitHub

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

Jump to bottom
Merged
mbrandenburger merged 1 commit into from
Dec 16, 2025
Merged

enhancement: implement multiton ListenerManager #1094

mbrandenburger merged 1 commit into from
Dec 16, 2025
+1,089 −219

Conversation

@SaidAltury-ibm
Copy link
Contributor

@SaidAltury-ibm SaidAltury-ibm commented Nov 28, 2025 •
edited by mbrandenburger
Loading

  • Introduced Provider that automatically manages ListenerManager lifecycle
  • Reuse manager instances per network:channel combination
  • listen() is now private and runs automatically in a background goroutine
  • Automatic cleanup when context is canceled or stream errors occur
  • increase test coverage

@SaidAltury-ibm SaidAltury-ibm added this to the 25Q4 milestone Nov 28, 2025
@SaidAltury-ibm SaidAltury-ibm self-assigned this Nov 28, 2025
@SaidAltury-ibm SaidAltury-ibm added the enhancement New feature or request label Nov 28, 2025
mbrandenburger
mbrandenburger reviewed Nov 28, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

@SaidAltury-ibm Thanks for submitting this PR.

mbrandenburger
mbrandenburger reviewed Dec 3, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

Hi @SaidAltury-ibm Thanks for the work. When looking at the new commit I had some thoughts ... let me share here :)

mbrandenburger
mbrandenburger reviewed Dec 8, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

Thanks @SaidAltury-ibm I have a few questions about the latest changes.

mbrandenburger
mbrandenburger reviewed Dec 11, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

Hi @SaidAltury-ibm Great progress on the impl.

mbrandenburger
mbrandenburger reviewed Dec 15, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

@SaidAltury-ibm Thanks for the latest updates. I am wondering if we can simplify the nlm by removing the ctx and cancelF. Please see my comments. THanks

mbrandenburger
mbrandenburger reviewed Dec 16, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

@SaidAltury-ibm Thanks! I think this last update simplified the code a lot. A few minors below.

mbrandenburger
mbrandenburger reviewed Dec 16, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

Beautiful! @SaidAltury-ibm A few nits ... I think we are almost done. Thanks

@SaidAltury-ibm SaidAltury-ibm force-pushed the SA-multiton-listenerManager branch 2 times, most recently from a16e0be to 1d8365c Compare December 16, 2025 17:59
@SaidAltury-ibm
enhancement: implement multiton ListenerManager
ce766ba 
Signed-off-by: Said Altury <Said.Altury@ibm.com>
@SaidAltury-ibm SaidAltury-ibm force-pushed the SA-multiton-listenerManager branch from 2b57ad8 to ce766ba Compare December 16, 2025 19:35
mbrandenburger
mbrandenburger approved these changes Dec 16, 2025
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

Well done!

@mbrandenburger mbrandenburger marked this pull request as ready for review December 16, 2025 20:03
@mbrandenburger mbrandenburger merged commit a042a1d into hyperledger-labs:main Dec 16, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbrandenburger mbrandenburger mbrandenburger approved these changes

Assignees

@SaidAltury-ibm SaidAltury-ibm

Labels

enhancement New feature or request

Projects

None yet

Milestone

25Q4

Development

Successfully merging this pull request may close these issues.

2 participants

@SaidAltury-ibm @mbrandenburger