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.

Sign up for GitHub

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

Jump to bottom

Testing futures #1129

Merged
merged 13 commits into from
Apr 8, 2024
Merged

Testing futures #1129

merged 13 commits into from
Apr 8, 2024

Conversation

SamBarker
Copy link
Member

Type of change

  • Refactoring

Description

Extract a promise factory so that we can apply better testing around timeouts (and potentially other common behaviours in future)

Additional Context

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
  • Make sure all Sonarcloud warnings are addressed or are justifiably ignored.
  • Update documentation
  • Reference relevant issue(s) and close them after merging
  • For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

SamBarker
SamBarker commented Apr 3, 2024
View reviewed changes
kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/FilterHandler.java
@@ -298,16 +298,11 @@ private <F extends FilterResult> F handleFilteringException(Throwable t, Decoded

private <F extends FilterResult> CompletableFuture<F> handleDeferredStage(DecodedFrame<?, ?> decodedFrame, CompletableFuture<F> future) {
Copy link
Member Author

Choose a reason for hiding this comment

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

decodedFrame is currently redundant in this PR as it was only passed in to work out if it was a request or response for logging purposes.

I'm conflicted about the removal of the logging.

Copy link
Member Author

Choose a reason for hiding this comment

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

AS discussed in slack, we've standardised on the logging level and will used the same message as the exception for logging. I've kept the usage of decodedFrame as I think it provides a better message

kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/PromiseFactory.java
var timeoutFuture = executorService.schedule(() -> {
final String message;
try {
message = exceptionMessageGenerator.call();
Copy link
Member Author

Choose a reason for hiding this comment

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

I also wonder if we should be passing in a metric to increment here as well (If only we had optional args ala Kotlin...)

@SamBarker SamBarker mentioned this pull request Apr 3, 2024
Closed
7 tasks
@SamBarker SamBarker marked this pull request as ready for review April 3, 2024 22:43
@robobario
Copy link
Contributor

Does this supercede #1125? We're now setting up cancellation for both usages of scheduled timeouts. The PR name/description makes it sound like refactoring only.

@SamBarker
Copy link
Member Author

Does this supercede #1125? We're now setting up cancellation for both usages of scheduled timeouts. The PR name/description makes it sound like refactoring only.

I guess it sorta does, I was expecting #1125 to have been merged before this was ready so it would just have been a refactoring.

tombentley
tombentley reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

A few minor comments, but I think the overall direction looks good.

kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/PromiseFactory.java Outdated Show resolved Hide resolved
kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/PromiseFactory.java Outdated
private final TimeUnit timeoutUnit;
private final Logger logger;

public PromiseFactory(ScheduledExecutorService executorService, long timeout, TimeUnit timeoutUnit, String loggerName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Why passing the logger name, rather than the logger itself?
  • Is it the case that we want the timeout to be scoped to the factory, rather than per-promise? In more sophisticated uses you might want to define a timeout that applies over a sequence of timeoutable actions, so you need to compute the remaining timeout and supply that for each subsequent action.

Copy link
Member Author

Choose a reason for hiding this comment

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

I passed the name rather than the logger as I can't think of another example where I've seen loggers passed around so it felt less surprising, happy to be persuaded otherwise.

Current usage calls for a factory wide logger so that's all I did for now, I also wasn't convinced by the api of passing the logger at the calls. Again happy to be persuaded otherwise.

I'm not sure we have use cases yet for request processing limits or global timeouts. Id rather refactor this when we do than try and design for a problem we don't have now.

kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/PromiseFactory.java Show resolved Hide resolved
kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/FilterHandler.java Outdated Show resolved Hide resolved
SamBarker added 10 commits April 5, 2024 16:12
@SamBarker
Introduce a PromiseFactory so we can consistently apply timeouts
8d2af7f 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Use PromiseFactory in handleDeferredStage
5c14210 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
FFS when will the formatters agree
113f671 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Inject a logger into the PromiseFactory so that we can log messages a…
745b951 
…bout timeouts.

rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Cleanup
b90ea1b 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Formatters fsk sake
6636ea6 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Refactor to expose the timeoutTask
5e042f5 
To enable testing

rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Log failing exceptionMessageGenerator invocations.
13ec669 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Add javadoc
43492e9 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Include timeout in logged messages.
b97f841 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Reduce promiseFactory to package visibility
ce2ec76 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker SamBarker requested a review from tombentley April 5, 2024 03:54
SamBarker added 2 commits April 5, 2024 17:06
@SamBarker
update message
856997d 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Add changelog entry
c26f661 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Apr 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

robobario
robobario approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@robobario robobario left a comment

Choose a reason for hiding this comment

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

LGTM

tombentley
tombentley approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Thanks @SamBarker

@SamBarker SamBarker merged commit e065419 into kroxylicious:main Apr 8, 2024
2 checks passed
@SamBarker SamBarker deleted the testingFutures branch April 8, 2024 05:27
@k-wall k-wall added this to the 0.5.1 milestone Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robobario robobario robobario approved these changes

@tombentley tombentley tombentley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Kroxylicious
Status: Done
Milestone
0.5.1
Development

Successfully merging this pull request may close these issues.
4 participants
@SamBarker @robobario @tombentley @k-wall