Mimic storage notifications using the HTTP API #1339

burmanm · 2023-08-10T08:18:59Z

This branch is based on the PR #1336

github-actions · 2023-08-10T08:19:12Z

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

Miles-Garnsey

Thanks for this @burmanm, it is good to see how your proposed notifications mechanism might work!

One Issue: can we possibly start putting some unit tests into this PR? I know higher level tests won't pass because we don't yet have the work done on management API, but it would be good to see unit tests if possible.

I've left a number of other comments (mostly questions) throughout the code. I'll also continue reading through to try to better understand how this will work.

Perhaps it will be clearer to me once some of the mgmt API stuff is complete and we can start to see the integration between the two more clearly.

src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java

src/server/src/main/java/io/cassandrareaper/management/http/models/Job.java

src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java

…ork without the actual client implementation Implement using apiClient the triggerRepair, getJobDetails, scheduler as well as add a simple test to ensure the state is managed correctly

Miles-Garnsey

Here's a preliminary review, I'm still working through this as I've been trying to get my PR merged today. I think maybe some more tests might be good, but not sure how hard they might be to implement given all of the concurrency stuff you have here.

Miles-Garnsey · 2023-08-24T08:16:37Z

src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020-2020 The Last Pickle Ltd
+ * Copyright 2023-2023 DataStax, Inc.


Hmmm, I think this should be

* Copyright 2020-2022 The Last Pickle Ltd * Copyright 2023 DataStax, Inc.

But I'm not a lawyer.

Checkstyle won't accept that format. In any case, this type of copyright info is not necessary in EU at least.

Same in AU, but we need to consider US.

Well, then that would be outside PR which would modify the checkstyle and copyright creation in the project. Right now the years are all over the place in every file.

src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java

Miles-Garnsey · 2023-08-24T08:53:28Z

...erver/src/test/java/io/cassandrareaper/management/http/HttpCassandraManagementProxyTest.java

+        RepairParallelism.PARALLEL,
+        Collections.singleton("table"), true, Collections.emptyList(), repairStatusHandler, Collections.emptyList(), 1);
+
+    assertEquals(123456789, repairNo);


Issue: can we make some assertions around which functions have been called and how many times?

Issue: can we please test the individual methods here? And also look at what happens under failure conditions?

Issue: can we make some assertions around which functions have been called and how many times?

Done in the next test. It's not what this test would test as it doesn't care if any method is called. This is intended to test that the maps have correct amount of entries (if someone refactors the repair endpoint for example, but forgets to insert correct places, then this test would fail and tell that info).

Issue: can we please test the individual methods here? And also look at what happens under failure conditions?

This is already calling the individual methods. triggerRepair & removeRepairStatusHandlers are the interesting methods this one calls directly.

Miles-Garnsey · 2023-08-24T08:54:32Z

...erver/src/test/java/io/cassandrareaper/management/http/HttpCassandraManagementProxyTest.java

+
+    RepairStatusHandler repairStatusHandler = Mockito.mock(RepairStatusHandler.class);
+
+    int repairNo = httpCassandraManagementProxy.triggerRepair(BigInteger.ZERO, BigInteger.ONE, "ks",


Question: can we figure out whether the beginToken/endToken is still the right way to call this function? From what I could see it seemed like the current underlying JMX call actually uses the RingRange, but I'd have to go back to confirm how this works.

Isn't that part of your PR? In this test those values mean nothing, they're simply ignored.

I think it only applies to Cassandra < 2.2 from memory, that's why I'm wondering if it is a good idea to merge this now, since additional work might yield a slightly different solution for parts of this PR.

I mean this PR doesn't care even if it's triggerRepair() and no parameters at all. That has no impact to functionality here.

Miles-Garnsey

Three comments to address, otherwise good to merge to the integration branch!

Miles-Garnsey · 2023-08-28T07:27:18Z

src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java

+  }
+
+  @VisibleForTesting
+  private void scheduleJobPoller(ScheduledExecutorService scheduler, int pollInterval) {


Let's remove this ScheduledExecutorService argument and call this.scheduler?

Miles-Garnsey · 2023-08-28T07:34:06Z

...erver/src/test/java/io/cassandrareaper/management/http/HttpCassandraManagementProxyTest.java

@@ -83,4 +109,98 @@ private <T extends Object> T jsonFromResourceFile(String filename, Class<T> claz
    return new ObjectMapper().readValue(
        this.getClass().getResource(filename).openStream(), clazz);
  }
+
+  @Test
+  public void testRepairHandlers() throws Exception {


Maybe consider more descriptive names for tests?

Miles-Garnsey · 2023-08-28T07:34:34Z

...erver/src/test/java/io/cassandrareaper/management/http/HttpCassandraManagementProxyTest.java

+    RepairStatusHandler workAroundHandler = (repairNumber, status, progress, message, cassandraManagementProxy)
+        -> callTimes.incrementAndGet();
+
+    //    RepairStatusHandler repairStatusHandler = mock(RepairStatusHandler.class);


Please remove comment.

* Add stubbed polling of job details from the mgmt-api. This will not work without the actual client implementation Implement using apiClient the triggerRepair, getJobDetails, scheduler as well as add a simple test to ensure the state is managed correctly * Merge test files after the rebase * Add a test to verify the behavior of the notifications polling * Address comments

burmanm mentioned this pull request Aug 11, 2023

Remove JMX specific interfaces from ICassandraManagementProxy #1336

Merged

Miles-Garnsey requested changes Aug 14, 2023

View reviewed changes

burmanm force-pushed the jobdetails_notifications branch from bdba052 to 1ccb72a Compare August 22, 2023 14:05

burmanm changed the base branch from master to integration/http-managementproxy August 22, 2023 14:06

burmanm marked this pull request as ready for review August 22, 2023 14:06

Add stubbed polling of job details from the mgmt-api. This will not w…

d01f3cd

…ork without the actual client implementation Implement using apiClient the triggerRepair, getJobDetails, scheduler as well as add a simple test to ensure the state is managed correctly

burmanm force-pushed the jobdetails_notifications branch from 1ccb72a to d01f3cd Compare August 23, 2023 09:05

burmanm added 2 commits August 23, 2023 13:56

Merge test files after the rebase

23fb839

Add a test to verify the behavior of the notifications polling

a1446bb

Miles-Garnsey requested changes Aug 25, 2023

View reviewed changes

Miles-Garnsey approved these changes Aug 28, 2023

View reviewed changes

Address comments

62226dd

burmanm merged commit e5bbf2f into integration/http-managementproxy Aug 28, 2023
2 checks passed

adejanovski linked an issue Aug 29, 2023 that may be closed by this pull request

Ensure Reaper can call the new Management API async notifications endpoints #1325

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mimic storage notifications using the HTTP API #1339

Mimic storage notifications using the HTTP API #1339

burmanm commented Aug 10, 2023

github-actions bot commented Aug 10, 2023

Miles-Garnsey left a comment

Miles-Garnsey left a comment

Miles-Garnsey Aug 24, 2023

burmanm Aug 25, 2023

Miles-Garnsey Aug 28, 2023

burmanm Aug 28, 2023

Miles-Garnsey Aug 24, 2023

Miles-Garnsey Aug 25, 2023

burmanm Aug 25, 2023

Miles-Garnsey Aug 24, 2023

burmanm Aug 25, 2023

Miles-Garnsey Aug 28, 2023

burmanm Aug 28, 2023

Miles-Garnsey left a comment

Miles-Garnsey Aug 28, 2023

Miles-Garnsey Aug 28, 2023

Miles-Garnsey Aug 28, 2023


		RepairStatusHandler repairStatusHandler = Mockito.mock(RepairStatusHandler.class);

		int repairNo = httpCassandraManagementProxy.triggerRepair(BigInteger.ZERO, BigInteger.ONE, "ks",

Mimic storage notifications using the HTTP API #1339

Mimic storage notifications using the HTTP API #1339

Conversation

burmanm commented Aug 10, 2023

github-actions bot commented Aug 10, 2023

Miles-Garnsey left a comment

Choose a reason for hiding this comment

Miles-Garnsey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miles-Garnsey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment