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

Add S3/MinIO support for application settings #2733

Closed
wants to merge 57 commits into from

Conversation

muuki88
Copy link
Contributor

@muuki88 muuki88 commented Oct 27, 2023

This PR adds support for an additional storage system to retrieve

  • application settings
  • stored impressions
  • stored requests
  • stored responses

Any S3 compatible system can be used.

Usage

It work's bascially the same as the file system settings, but the files are on S3. What we found, works really well, is using the ad unit path of GAM for the stored impression ID as this maps nicely to paths.

Credits

Credits goe to @0xlee for the intitial idea and @rmattis for the actual implementation.

pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@muuki88
Copy link
Contributor Author

muuki88 commented Jan 25, 2024

@And1sS I hope I resolved all the issues. Do you mind taking a look 🧐☺️

@muuki88 muuki88 requested a review from And1sS February 12, 2024 22:29
pom.xml Outdated Show resolved Hide resolved
@muuki88 muuki88 requested a review from And1sS March 4, 2024 12:22
@SerhiiNahornyi
Copy link
Collaborator

@muuki88 Java Ci failed, due to checkstyle

@SerhiiNahornyi
Copy link
Collaborator

Java CI failed because of checkstyle

Error:  /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/settings/service/S3PeriodicRefreshService.java:16:8: Unused import - software.amazon.awssdk.core.BytesWrapper. [UnusedImports]
Error:  /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/settings/service/S3PeriodicRefreshService.java:25:8: Unused import - java.util.ArrayList. [UnusedImports]

@SerhiiNahornyi
Copy link
Collaborator

SerhiiNahornyi commented Jul 26, 2024

Again, checkstyle :)

Usually, to avoid this, before pushing, we run the command mvn clean package -f extra in the root of the project. It performs the same checks that Java CI does.

Error:  /home/runner/work/prebid-server-java/prebid-server-java/src/test/java/org/prebid/server/settings/service/S3PeriodicRefreshServiceTest.java:32:15: Unused import - java.util.Collections.emptyList. [UnusedImports]
Error:  /home/runner/work/prebid-server-java/prebid-server-java/src/test/java/org/prebid/server/settings/service/S3PeriodicRefreshServiceTest.java:33:15: Unused import - java.util.Collections.emptyMap. [UnusedImports]
Error:  /home/runner/work/prebid-server-java/prebid-server-java/src/test/java/org/prebid/server/settings/service/S3PeriodicRefreshServiceTest.java:34:15: Unused import - java.util.Collections.singletonList. [UnusedImports]

@SerhiiNahornyi
Copy link
Collaborator

Some tests inside Java CI failed

Errors: 
Error:    S3PeriodicRefreshServiceTest.initializeShouldMakeOneInitialRequestAndTwoScheduledRequestsWithParam:95->createAndInitService:165 » NullPointer Cannot invoke "io.vertx.core.impl.ContextInternal.promise()" because "context" is null
Error:    S3PeriodicRefreshServiceTest.initializeShouldMakeOnlyOneInitialRequestIfRefreshPeriodIsNegative:105->createAndInitService:165 » NullPointer Cannot invoke "io.vertx.core.impl.ContextInternal.promise()" because "context" is null
Error:    S3PeriodicRefreshServiceTest.shouldCallSaveWithExpectedParameters:82->createAndInitService:165 » NullPointer Cannot invoke "io.vertx.core.impl.ContextInternal.promise()" because "context" is null
Error:    S3PeriodicRefreshServiceTest.shouldUpdateTimerAndErrorMetric:129->createAndInitService:165 » NullPointer Cannot invoke "io.vertx.core.impl.ContextInternal.promise()" because "context" is null
Error:    S3PeriodicRefreshServiceTest.shouldUpdateTimerMetric:115->createAndInitService:165 » NullPointer Cannot invoke "io.vertx.core.impl.ContextInternal.promise()" because "context" is null

@muuki88
Copy link
Contributor Author

muuki88 commented Aug 17, 2024

The tests fail due to the requested vertx.getOrCreateContext refactoring. I couldn't find any other usage in the codebase of this method call and thus no example on how to mock this. Also searching the internet I wasn't able to figure out how to stub this method call.

java.lang.NullPointerException: Cannot invoke "io.vertx.core.impl.ContextInternal.promise()" because "context" is null

	at io.vertx.core.Future.fromCompletionStage(Future.java:623)
	at org.prebid.server.settings.service.S3PeriodicRefreshService.listFiles(S3PeriodicRefreshService.java:141)
	at org.prebid.server.settings.service.S3PeriodicRefreshService.getFileContentsForDirectory(S3PeriodicRefreshService.java:125)
	at org.prebid.server.settings.service.S3PeriodicRefreshService.fetchStoredDataResult(S3PeriodicRefreshService.java:95)
	at org.prebid.server.settings.service.S3PeriodicRefreshService.initialize(S3PeriodicRefreshService.java:82)
	at org.prebid.server.settings.service.S3PeriodicRefreshServiceTest.createAndInitService(S3PeriodicRefreshServiceTest.java:165)
	at org.prebid.server.settings.service.S3PeriodicRefreshServiceTest.initializeShouldMakeOnlyOneInitialRequestIfRefreshPeriodIsNegative(S3PeriodicRefreshServiceTest.java:105)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

@CTMBNara
Copy link
Collaborator

This works, but it's kinda strange, because it just swallows the exception.

The point of this initializePromise is to check if that service was started properly. If it can't start, then there's no point in starting up prebid server at all, because all configuration is missing.

While this "fixes" the issue, it allows prebid server to start up in a useless state.

WDYT?

Yeah, you are right. Thanks that you accidentally found it :) Bug-fix: #3385

Regarding your question about "how to mock vertx.getOrCreateContext() call": see CircuitBreakerSecuredGeoLocationServiceTest.class on how to mock vertx environment for tests.
Also, given that we have fixed the underlying issues, we need to revert this commit.

@muuki88
Copy link
Contributor Author

muuki88 commented Aug 20, 2024

Regarding your question about "how to mock vertx.getOrCreateContext() call": see CircuitBreakerSecuredGeoLocationServiceTest.class on how to mock vertx environment for tests. Also, given that we have fixed the underlying issues, we need to revert this commit.

I did it a little bit differently to keep the assertions being made

@muuki88
Copy link
Contributor Author

muuki88 commented Aug 25, 2024

Sorry for my confusion. Should I use vertx.getOrCreateContext() or not 😅?

I guess this is the only thing left. And the adjustments to tests, depending on the decision.

@CTMBNara
Copy link
Collaborator

Sorry for my confusion. Should I use vertx.getOrCreateContext() or not 😅?

I guess this is the only thing left. And the adjustments to tests, depending on the decision.

Better to use

@Compile-Ninja
Copy link
Collaborator

@muuki88, Thank you so much for your hard work and contributions on PR #2733. Your efforts laid a strong foundation, and we genuinely appreciate the time and energy you invested in this.

Since we needed to move forward and complete the work, we created a new PR (PR #3418) based on your original changes. The new PR is now finished, and we’re just waiting for your approval to proceed.

We’re truly grateful for your contributions, and your work was instrumental in getting us to this point. We would greatly appreciate it if you could take a moment to review and approve the new PR when you have a chance.

Thanks again!

@muuki88
Copy link
Contributor Author

muuki88 commented Sep 3, 2024

Hi @Compile-Ninja

That means a lot ❤️ thank you. I'll try to review it as soon as possible. Currently I'm on vacation

@Compile-Ninja
Copy link
Collaborator

Duplication of #3418

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.

9 participants