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

Allow MongoDB to be unavailable #827

Closed
wants to merge 3 commits into from
Closed

Conversation

myieye
Copy link
Contributor

@myieye myieye commented May 28, 2024

I don't love how I've implemented this.
But, LF classic is a seperate service so it seems reasonable to make it optional and have less development dependencies.
I'm not sure how to make it better.

If anyone thinks this is bad, silly or unnecessary that's fine with me.
Or is there a better way to do this?

Copy link

github-actions bot commented May 28, 2024

C# Unit Tests

71 tests  +14   71 ✅ +14   9s ⏱️ -1s
12 suites + 1    0 💤 ± 0 
 2 files   ± 0    0 ❌ ± 0 

Results for commit e889b07. ± Comparison against base commit 8c03571.

This pull request removes 3 and adds 17 tests. Note that renamed tests count towards both.
Testing.LexCore.CrdtCommitTests ‑ CanRoundTripCommitChanges
Testing.LexCore.CrdtCommitTests ‑ CanSaveCrdtCommit
Testing.LexCore.CrdtCommitTests ‑ TypePropertyShouldAlwaysBeFirst
Testing.LexCore.CrdtServerCommitTests ‑ CanRoundTripCommitChanges
Testing.LexCore.CrdtServerCommitTests ‑ CanSaveServerCommit
Testing.LexCore.CrdtServerCommitTests ‑ TypePropertyShouldAlwaysBeFirst
Testing.LexCore.ProjectCodeTests ‑ InvalidCodesThrows(code: "!")
Testing.LexCore.ProjectCodeTests ‑ InvalidCodesThrows(code: "#")
Testing.LexCore.ProjectCodeTests ‑ InvalidCodesThrows(code: "-not-start-with-dash")
Testing.LexCore.ProjectCodeTests ‑ InvalidCodesThrows(code: "../hacker")
Testing.LexCore.ProjectCodeTests ‑ InvalidCodesThrows(code: "/hacker")
Testing.LexCore.ProjectCodeTests ‑ InvalidCodesThrows(code: "_____deleted_____")
Testing.LexCore.ProjectCodeTests ‑ InvalidCodesThrows(code: "_____temp_____")
…

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

there's some weird stuff going on in the IsAvailable call, though I might just rewrite it to use a long delay before checking again so we don't check every single time.

Comment on lines 27 to 31
_isAvailable = await Task.Run(() =>
{
return _mongoDatabase.RunCommandAsync((Command<BsonDocument>)"{ping:1}").Wait(100);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uh, I'm not sure what you're trying to do here, but I don't think this is what you want. This will attempt to run RunCommandAsync synchronously (via the Wait) and throw if it doesn't finish in time, meanwhile you're awaiting a task that runs that async code as sync. You might want to just do this:

Suggested change
_isAvailable = await Task.Run(() =>
{
return _mongoDatabase.RunCommandAsync((Command<BsonDocument>)"{ping:1}").Wait(100);
});
_isAvailable = await _mongoDatabase.RunCommandAsync((Command<BsonDocument>)"{ping:1}")
.WaitAsync(TimeSpan.FromMilliseconds(100));

this will throw if there's not a response within 100ms, so that still might not be what you want.

Copy link
Contributor Author

@myieye myieye May 29, 2024

Choose a reason for hiding this comment

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

No, Wait(100) never throws. It returns a boolean indicating whether the command finished within the timeout:

// Returns:
// true if the System.Threading.Tasks.Task completed execution within the allotted
// time; otherwise, false.

Copy link
Collaborator

@hahn-kev hahn-kev May 29, 2024

Choose a reason for hiding this comment

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

ahh ok, I forget some of them throw on timeout. It would still probably be better to catch exceptions and not use Wait as that's intended as a last resort when calling an Async method from a sync context. I'd use WaitAsync for that instead, although it throws a timeout exception when hitting the timeout.

@myieye myieye force-pushed the feat/make-mongo-optional branch from 408c086 to 73e44fd Compare May 29, 2024 15:21
@myieye myieye marked this pull request as draft May 29, 2024 15:21
@myieye
Copy link
Contributor Author

myieye commented Jun 4, 2024

This doesn't actually seem to work currently. I'm not sure what I did in my initial testing, maybe I set _isAvailable to false explicitly while I was testing or something. Anyway, if Mongo is not up then I just get:

2024-06-04 11:43:48       1 CompositeServerSelector{ Selectors = ReadPreferenceServerSelector{ ReadPreference = { Mode : Primary } }, LatencyLimitingServerSelector{ AllowedLatencyRange = 00:00:00.0150000 }, OperationsCountServerSelector } 1 aggregate { ClusterId : "1", Type : "Unknown", State : "Disconnected", Servers : [{ ServerId: "{ ClusterId : 1, EndPoint : "Unspecified/host.docker.internal:27017" }", EndPoint: "Unspecified/host.docker.internal:27017", ReasonChanged: "ServerInitialDescription", State: "Disconnected", ServerVersion: , TopologyVersion: , Type: "Unknown", LastHeartbeatTimestamp: null, LastUpdateTimestamp: "2024-06-04T09:43:47.9909070Z" }] } Waiting for suitable server to become available 29997

And it has nothing to do with my Ping/wait/etc.

@myieye myieye closed this Jun 4, 2024
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