-
Notifications
You must be signed in to change notification settings - Fork 58
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
Implement Lame Duck Mode Event Handler #716
base: main
Are you sure you want to change the base?
Conversation
edda667
to
15f7ac2
Compare
set | ||
{ | ||
var current = Interlocked.CompareExchange(ref _writableServerInfo, null, null); | ||
if (current?.LameDuckMode == false && value?.LameDuckMode == true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might encounter another ld server. probably better not to check a state change but fire every time.
@pzajaczkowski thanks for the PR. long overdue that issue ;)
I think that's fine.
Potentially passing the host might be a interesting. EDIT: What do you think about passing |
@@ -505,6 +505,32 @@ public async Task ReconnectOnOpenConnection_ShouldDisconnectAndOpenNewConnection | |||
openedCount.ShouldBe(1); | |||
disconnectedCount.ShouldBe(1); | |||
} | |||
|
|||
[Fact] | |||
public async Task LameDuckModeActivated_EventHandlerShouldBeInvokedWhenInfoWithLDMRecievied() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this test won't run on Windows due to lack of posix signals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can do this instead...
// var command = $"$SYS.REQ.SERVER.{serverId}.LDM";
RequestAsync(command , ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @darkwatchuk is right, with LDM API introduced this is definately an easier way to test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pzajaczkowski are you ok to change the test to use the API above?
@@ -109,6 +109,8 @@ public NatsConnection(NatsOpts opts) | |||
|
|||
public event AsyncEventHandler<NatsMessageDroppedEventArgs>? MessageDropped; | |||
|
|||
public event AsyncEventHandler<NatsEventArgs>? LameDuckModeActivated; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pzajaczkowski let's also pass a copy of server info host Uri here so the application can be aware of which host etc.
Resolves #23
I had two ideas for approaching this issue:
WritableServerInfo
property and add setter, that will check if LDM has changed totrue
NatsConnection
and invoke it directly fromNatsReadProtocolProcessor
after recieving INFO from server with LDM.I decided to go with first option.
There is one thing i am not sure of: should anything be passed as argument to the
LameDuckModeActivated
event? Currently, it is an empty string.