CASSSIDECAR-180 - Changes to add endpoint for stream stats #166
Conversation
|
|
||
| private StreamProgressStats computeStats(List<StreamState> streamStates) | ||
| { | ||
| List<SessionInfo> sessions = streamStates.stream().map(s -> s.sessions()).flatMap(Collection::stream).collect(Collectors.toList()); |
There was a problem hiding this comment.
I understand this is by no means as hot path as the ones the dev thread was referring to, but I wonder if we should keep that choice in sidecar of avoiding streams in non test code.
Same for the other streams on the PR
There was a problem hiding this comment.
Thanks for sharing. This requires a broader discussion around which of these requirements apply to the sidecar in general. One way of looking at it is for sidecar to inherit the model that Cassandra adopts (although, seems like the thread did not arrive at an action item).
The alternative, which I am inclined to, is to look closer into the specific sidecar behavior/functionality we are optimizing for. For control plane use-cases, which involve APIs that we query to determine node states, we should be fine with the current usage, with readability and maintainability in mind.
For bulk-analytics, CDC and resource sensitive/large-dataset use-cases, we should rely on perf benchmarks and review the streams usage (on similar lines to the hot-path discussion in C*).
|
|
||
| public ProgressInfo(CompositeData data) | ||
| { | ||
|
|
| } | ||
|
|
||
| /* Across all sessions | ||
| * private final long sendProgress; from "sent to" |
There was a problem hiding this comment.
| * private final long sendProgress; from "sent to" | |
| private final long sendProgress; from "sent to" |
| { | ||
| String streamStatsResponseAsString = "{\"operationMode\":\"NORMAL\"," + | ||
| "\"streamProgressStats\":{\"totalFilesToReceive\":7," + | ||
| "\"totalFilesReceived\":7,\"totalBytesToReceive\":15088," + |
There was a problem hiding this comment.
These numbers make me wonder if it would be worth to consider adding Quick Theories testing library to sidecar like we have in Cassandra. I could see the case of leaving that out of this PR, but it would definitely increase coverage.
| assertThat(progressStats.totalFilesToSend()).isNotNull(); | ||
| assertThat(progressStats.totalFilesReceived()).isNotNull().isEqualTo(progressStats.totalFilesToReceive()); | ||
| assertThat(progressStats.totalFilesSent()).isNotNull(); | ||
|
|
| @@ -372,6 +374,9 @@ public Router vertxRouter(Vertx vertx, | |||
| router.get(ApiEndpointsV1.LIST_OPERATIONAL_JOBS_ROUTE) | |||
| .handler(listOperationalJobsHandler); | |||
|
|
|||
| router.get(ApiEndpointsV1.STREAM_STATS_ROUTE) | |||
There was a problem hiding this comment.
If (hopefully when :-P ) this PR gets merged, I would love to see the new API definition updated there as well. Let's work together to make that happen.
| if (streamProgress.totalFilesToReceive() > 0) | ||
| { | ||
| hasStats.set(true); | ||
| if (streamProgress.totalFilesToReceive() == streamProgress.totalFilesReceived() && |
| { | ||
| StreamingStatsTestModule.streamingStatsSupplier = () -> { | ||
| StreamStatsResponse response = new StreamStatsResponse("NORMAL", | ||
| new StreamProgressStats(7, 7, 1024, 1024, 0, 0, 0, 0)); |
There was a problem hiding this comment.
Same comment with quick theories testing library.
These changes are part of the effort to introduce bespoke Sidecar APIs to support key operational functionality currently managed through nodetool commands.
These changes specifically return the stream-stats data in a sidecar endpoint.
Changes
nodetool netstatsdataset