-
Notifications
You must be signed in to change notification settings - Fork 8
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
Exception Factory via separate PR as requested #178
Exception Factory via separate PR as requested #178
Conversation
…lled too many times.
…ponse. Refuse work when load too high.
…nhance performance.
…ption stacktraces configurable by application.
Related: SDCISA-15833, swisspost#170, swisspost#177, swisspost/vertx-rest-storage#186, swisspost/gateleen#577
Related: SDCISA-15833, swisspost#170, swisspost#177, swisspost/vertx-rest-storage#186, swisspost/gateleen#577
return; | ||
} | ||
Message<JsonObject> msg = ev.result(); | ||
JsonObject body = msg.body(); | ||
String status = body.getString(STATUS); | ||
if (!OK.equals(status)) { | ||
onDone.accept(new Exception("Unexpected status " + status), null); | ||
Throwable ex = exceptionFactory.newException("Unexpected status " + status); | ||
assert ex != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have to assert not Null here? If we don't trust the implementation of the Interface we would have to check everywhere we use the RedisQuesExceptionFactory
log.info("Enabling http request handler: {}", modConfig.getHttpRequestHandlerEnabled()); | ||
if (modConfig.getHttpRequestHandlerEnabled()) { | ||
if (modConfig.getHttpRequestHandlerPort() != null && modConfig.getHttpRequestHandlerUserHeader() != null) { | ||
RedisquesHttpRequestHandler handler = new RedisquesHttpRequestHandler(vertx, modConfig, queueStatisticsCollector, dequeueStatisticCollector); | ||
var handler = new RedisquesHttpRequestHandler( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations, I think you are the first one in the gateelen&friends universe using var instead of the type :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is my assumption correct that this is a request to replace them by explicit type names? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's OK for me :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes all look good. Before merging this to develop I would suggest to replace all occurrences of logging new Exception(...)
with the new factory method
I would prefer to not do this within this PR directly. As it will mess up other WorkInProgress branches and PRs which are already on track. Leading to time consuming merge conflicts there. Maybe I can fulfill this request in the sibbling PRs in gateleen and/or RestStorage. But in RedisQues there are other branches in progress. |
No description provided.