Skip to content

New MembershipManager simplification opportunities (Daves unaddressed feedback) #4743

Open
@toger5

Description

@toger5

This issue collects Daves feedback that has not yet been addressed.
It helps us tracking those points once we decide it is the right time to make resources to simplify the new membership manager (probably in the context of removing the SendFirstDelayedEvent/SendMainDelayedEvent complexity)

It took a lot of staring at this class to understand it. A number of points:

It's clearly trying hard to be a generic timer manager so it feels a bit odd that the callback has 'membership' in the name rather than just, 'onTimeout' or something. Same with hasMemberStateEvent which seems to live in here but just have the MembershipManager itself gut-wrench into here to change it.

This function is an absolute behemoth and I'd be surprised if sonar allows it under the function complexity gate. You could have individual callbacks for the various actions, or use one central one but just have the switch statement defer to individual private functions: they don't appear to share any variables.

private maxDelayExceededErrorHandler(error: unknown): boolean {

Feels like there could be a better name for this function although handleThingThatMayBeADelayExceededError feels a little long. Also, the things calling it still have to do something different depending on whether the thing turned out to be the error in question or not, so I'd be inclined to just separate the two concepts (ie. "is this thing a delay exceeded error" and "handle the error").

IMPORTANT:

I can't quite wrap my head around why the new actions need to be added to a separate array and then added on to the main one later by the loop function.

Metadata

Metadata

Assignees

No one assigned

    Labels

    T-TaskTasks for the team like planning

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions