[WIP] Add interfaces for refactoring work #1276
Conversation
Qodana Community for JVM1 new problem were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/[email protected]
with:
upload-result: trueContact Qodana teamContact us at [email protected]
|
aaron-congo
force-pushed
the
refactoring-wip
branch
from
184b68a to
528af13
Compare
aaron-congo
force-pushed
the
refactoring-wip
branch
from
528af13 to
9ebeb8a
Compare
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
| import software.amazon.jdbc.plugin.customendpoint.CustomEndpointInfo; | ||
|
|
||
| public interface StorageService { |
There was a problem hiding this comment.
boolean exists(String itemCategory, Object key);
| * | ||
| * @param event the event to publish. | ||
| */ | ||
| void publish(Object event); |
There was a problem hiding this comment.
It's better to replace Object with a simple interface message interface (even if such interface is empty)
There was a problem hiding this comment.
I was wondering about that. I didn't add it because I think subscribers will have to cast the event whether there's an interface or not, since our events will be pretty generic. But I can see how it might be useful so I can add it.
|
|
||
| package software.amazon.jdbc.util.monitoring; | ||
|
|
||
| public enum MonitorState { |
There was a problem hiding this comment.
Does that indicate a monitor that has been instantiated but never run? With the current design the monitor service doesn't provide a method to submit a monitor without running it, we just have the "runIfAbsent" method which would automatically create and run the monitor. So I'm not sure if we'd ever have a monitor in this state.
| private long lastUsedTimeNs; | ||
| private @Nullable Throwable exception; | ||
|
|
||
| public MonitorStatus(MonitorState state, long lastUsedTimeNs) { |
There was a problem hiding this comment.
We might need to make this class immutable.
There was a problem hiding this comment.
I was also wondering about that. If we make it immutable the monitor will have to create a new MonitorStatus object each time it needs to update its state, which could be quite frequent since it will be regularly updating the lastUsedTime field. I was also wondering if we should get rid of the MonitorStatus object and just have its fields directly in the Monitor class. If we did that we would have to add getters to the Monitor interface (getMonitorState, getLastUsedTime, getException).
|
|
||
| void stop(); | ||
|
|
||
| MonitorStatus getStatus(); |
There was a problem hiding this comment.
There's an exception as part of MonitorStatus. Is it an unhandled exception that stops/breaks a monitor?
There was a problem hiding this comment.
That was my intention yes, it would be a reference to an exception that caused a monitor to unexpectedly stop.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.