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

gate: add named_gate #2688

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

gate: add named_gate #2688

wants to merge 3 commits into from

Conversation

bhalevy
Copy link
Member

@bhalevy bhalevy commented Mar 17, 2025

Allow specifying a name for the gate when created using a named exception factory,
to help debugging gate_closed_exception.

@bhalevy
Copy link
Member Author

bhalevy commented Mar 17, 2025

Intended use case: scylladb/scylladb#23329

@xemul
Copy link
Contributor

xemul commented Mar 19, 2025

Same problem for semaphores is solved with the help of "named" exception factory (59a6a47). It's worth making gates and semaphores symmetrical in this sense

@bhalevy
Copy link
Member Author

bhalevy commented Mar 20, 2025

In v2 (380d057):

  • rebased
  • used exception factory to distinguish between gate and named_gate.
    • added a templated basic_gate(T value) constructor as a helper to construct named gate as named_gate("foo")

Copy link
Contributor

@xemul xemul left a comment

Choose a reason for hiding this comment

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

LGTM, but

  1. "gate: add named_gate" patch comment should be updated (patch title should probably be updated too)
  2. CI shows compilation failures

bhalevy added 3 commits March 24, 2025 13:04
Signed-off-by: Benny Halevy <[email protected]>
Prepare for adding named_gate using
a named gate_closed_exception_factory.

Signed-off-by: Benny Halevy <[email protected]>
By specializing basic_gate using
named_gate_exception_factory.

And add respective unit test.

Signed-off-by: Benny Halevy <[email protected]>
@bhalevy bhalevy changed the title gate: add optional name gate: add named_gate Mar 24, 2025
@bhalevy
Copy link
Member Author

bhalevy commented Mar 24, 2025

In v3 (7c31e6f):

  • rebased
  • extracted gate: templatize class basic_gate
  • renamed patch and edited git commit message
  • also this PR's title and description
  • fixed build issues in debug mode

@bhalevy bhalevy requested a review from xemul March 24, 2025 14:44
@bhalevy
Copy link
Member Author

bhalevy commented Mar 24, 2025

FWIW, the strong named_gate type causes a lot of churn since it is not derived from gate :-/

@bhalevy bhalevy marked this pull request as draft March 25, 2025 06:37
@bhalevy
Copy link
Member Author

bhalevy commented Mar 25, 2025

@xemul please see alternative design here: bhalevy@4907d7d
This one causes much less churn as named_gate is derived from gate in this model and other than using named_gate for defining members/variables so they can constructed with a name, they are indistinguishable from regular, unnamed, gates.

@xemul
Copy link
Contributor

xemul commented Mar 26, 2025

FWIW, the strong named_gate type causes a lot of churn since it is not derived from gate :-/

If this whole churn is isolated in gate.hh, I'd still go this way, it looks more solid and self-contained to me

@bhalevy
Copy link
Member Author

bhalevy commented Mar 27, 2025

FWIW, the strong named_gate type causes a lot of churn since it is not derived from gate :-/

If this whole churn is isolated in gate.hh, I'd still go this way, it looks more solid and self-contained to me

No, it requires to change current user code that use gate::holder to be modified to named_gate::holder, since named_gate is strongly distinguishable from gate.
This it not the case with semaphores since there, sempahore_units is an independent template, allowing get_units to operate on any specialized semaphore with no change to user code.
But in contrast, gate::holder is defined as a nested type.

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.

2 participants