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

S3 application settings #3418

Merged
merged 63 commits into from
Sep 9, 2024
Merged

S3 application settings #3418

merged 63 commits into from
Sep 9, 2024

Conversation

CTMBNara
Copy link
Collaborator

@CTMBNara CTMBNara commented Sep 3, 2024

This PR is based on the work contributed by @muuki88 in PR #2733. We deeply appreciate @muuki88’s initial efforts, which greatly helped in shaping this final implementation. 🙌💪❤️

rmattis and others added 30 commits October 27, 2023 20:50
And1sS
And1sS previously approved these changes Sep 3, 2024
@Compile-Ninja
Copy link
Collaborator

@muuki88 When you have time, could you please approve this change? 🙂


// then

result.onComplete(context.failing(cause -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the test timeout or give a proper error message if it wouldn't fail?

Copy link
Collaborator Author

@CTMBNara CTMBNara Sep 4, 2024

Choose a reason for hiding this comment

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

Yes, there is a default timeout in VertxTestContext.

vertx.setPeriodic(refreshPeriod, ignored -> fetchStoredDataResult(clock.millis(), MetricName.update));
}

initializePromise.tryComplete();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still favouring the solution where the promise is completed with the first fetch result.

Failing to fetch stored impressions/requests is a critical sign that the new instance shouldn't startup.

And it should also open a port until the initial data is loaded.

@muuki88
Copy link
Contributor

muuki88 commented Sep 3, 2024

I took a look on my smartphone with the Github App 😄

The only thing is the refresh service initialize promise. I'm convinced that it should resolve the promise with the initial fetch result

@CTMBNara CTMBNara requested review from muuki88 and And1sS September 4, 2024 13:58
@muuki88
Copy link
Contributor

muuki88 commented Sep 4, 2024

Looks good 🥳 Thanks for polishing this up and make it ready to go.

@Compile-Ninja Compile-Ninja merged commit 4fac10c into master Sep 9, 2024
5 checks passed
@Compile-Ninja Compile-Ninja deleted the s3-application-settings/copy branch September 9, 2024 11:28
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.

6 participants