Small improvements #407
Small improvements #407
Conversation
There was a problem hiding this comment.
Looks good. Just some minor comments.
My main impression from this PR is a reminder that we really ought to get rid of the usage of "backend" throughout the codebase and docs and replace it with "Trino cluster" and "cluster" as necessary in the context. We agreed on doing this in a dev sync and just have not gotten around to it.. maybe we should file an issue so we dont forget.
|
|
||
| scheduledFuture = scheduledExecutor.scheduleAtFixedRate(() -> { | ||
| try { | ||
| log.info("Getting the stats for the active clusters"); |
There was a problem hiding this comment.
| log.info("Getting the stats for the active clusters"); | |
| log.info("Getting stats for all active clusters"); |
|
Filed the issue now - #408 |
| scheduledFuture.cancel(true); | ||
| scheduledExecutor.shutdown(); |
There was a problem hiding this comment.
I think scheduledFuture can be removed if we use scheduledExecutor.shutdownNow().
| catch (Exception e) { | ||
| log.error(e, "Error with monitor task"); | ||
| } | ||
| if (clusterStatsObservers != null) { |
There was a problem hiding this comment.
The requireNonNull added above should have eliminated this condition.
| } | ||
| return null; |
There was a problem hiding this comment.
This change from using null to throwing exceptions for error handling.
The null checks need to be updated accordingly.
| yield null; | ||
| } | ||
| default -> null; | ||
| }; | ||
| } | ||
| catch (IOException e) { | ||
| e.printStackTrace(); | ||
| throw new UncheckedIOException(e); |
There was a problem hiding this comment.
I'm a bit confused on the error handling.
Why mix null and throw exception for error handling?
|
@wendigo .. gentle ping ;-) |
|
@mosabua ? :) |
|
How should we proceed. Should we merge it and create some follow-up PR? |
|
@oneonestar I'll go back to this PR later this week :) |
Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:
*