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

Drive health #156

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Drive health #156

merged 8 commits into from
Jan 16, 2024

Conversation

johnml1135
Copy link
Collaborator

@johnml1135 johnml1135 commented Jan 11, 2024

Resolves #141 and sillsdev/serval#243.


This change is Reviewable

* upgrade to net8.0
* Add health check endpoint to GRPC engine proto (from serval)
* Combine health reports into rich data
@johnml1135 johnml1135 requested a review from ddaspit January 11, 2024 18:08
Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johnml1135)


src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs line 224 at r1 (raw file):

    public static IMachineBuilder AddMongoDataAccess(this IMachineBuilder builder, string? connectionString = null)
    {
        connectionString ??= builder.Configuration!.GetConnectionString("Mongo");

Please avoid using the null-forgiving operator when possible. Throw an exception if necessary.


src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs line 375 at r1 (raw file):

            string EnginesDir = builder.Configuration
                .GetSection(SmtTransferEngineOptions.Key)!

I don't think that the null-forgiving operators are needed here.


src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs line 378 at r1 (raw file):

                .GetValue<string>("EnginesDir")!;

            string driveLetter = Path.GetPathRoot(EnginesDir)![..1];

Please avoid using the null-forgiving operator when possible.


src/SIL.Machine.AspNetCore/Services/ClearMLAuthenticationService.cs line 13 at r1 (raw file):

    // to know well ahead of time if something is wrong.
    private static readonly TimeSpan RefreshPeriod = TimeSpan.FromSeconds(3600);
    private string? _authToken = "";

I don't think it is ever possible for _authToken to be null.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

@johnml1135
Copy link
Collaborator Author

src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs line 224 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Please avoid using the null-forgiving operator when possible. Throw an exception if necessary.

Does that meet your expectations?

@johnml1135
Copy link
Collaborator Author

src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs line 378 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Please avoid using the null-forgiving operator when possible.

Fixed.

@johnml1135
Copy link
Collaborator Author

src/SIL.Machine.AspNetCore/Services/ClearMLAuthenticationService.cs line 13 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't think it is ever possible for _authToken to be null.

I revised the logic a bit to handle edge cases better. Specifically, if we can't get the auth token when first starting up, we should crash out. If refreshing fails, we should keep laboring on as long as possible.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

@johnml1135 johnml1135 merged commit e0367cc into master Jan 16, 2024
2 of 4 checks passed
@ddaspit ddaspit deleted the drive_health branch January 30, 2024 15:59
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.

Add health check for SMT engine disk storage
2 participants