CASSSIDECAR-162: Sidecar endpoint to GET sstable's preemptive interval value #152

skoppu22 · 2024-12-03T14:09:52Z

A new endpoint is developed in Sidecar, to retrieve value of C*'s sstable_preemptive_open_interval value. Currently end users have to invoke C*'s JMX call, which requires C* superuser role to invoke any JMX operation. With this change, end users can just use this endpoint, which abstracts underlying C*'s version dependencies/changes. We can enable authorization checks for these endpoints once we have authorization developed in Sidecar

Circle CI run: https://app.circleci.com/pipelines/github/skoppu22/cassandra-sidecar?branch=jmxep1

Integration test using dtest Sidecar client changes

server/src/main/java/org/apache/cassandra/sidecar/metrics/ServerMetrics.java

bbotella · 2024-12-03T23:30:29Z

server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java

+            throw cassandraServiceUnavailable();
+        }
+
+


Extra space

bbotella · 2024-12-03T23:30:59Z

server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java

+        StorageOperations storageOperations = delegate == null ? null : delegate.storageOperations();
+        if (storageOperations == null)
+        {
+            throw cassandraServiceUnavailable();


Do we want to add some text to this error?

cassandraServiceUnavailable currently says "Cassandra service is unavailable". The same message goes back in client's response. As this is an internal error, adding further info may leak internal info to the client. We use the same cassandraServiceUnavailable almost everywhere without adding further details, must be the same reason I am assuming.

Any other kind of exception is caught in AbstractHandler's handle() and logged and updated in the response.

This code no longer exists, Sidecar code was refactored to check for these in delegate getters

server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java

bbotella · 2024-12-03T23:39:50Z

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java

+    // Endpoint to retrieve sstable's preemptiveOpenInterval value.
+    // Value returned is in MB, may return negative value when disabled
+    private static final String SSTABLE = "/sstable";
+    public static final String SSTABLE_PREEMPTIVE_OPEN_INTERVAL_ROUTE = API_V1 + CASSANDRA + SSTABLE +


I know we are reusing the getSsTablePreemtiveOpenIntervalInMB JMX operation, but, from a pure API standpoint, we are omitting the MB part in the endpoint, but returning it as part of the response (SSTablePreemptiveOpenIntervalInMB). My suggestion for this would be to either:

We add the MB part to the endpoint (/preemptive-open-interval-in-mb)
Or

We just support an optional unit filtering parameter in the url, that can be extended in the future with other unit types if needed. That way, the url would end in /preemptive-open-interval?unit=mb (remember that the unit is optional with MB as a default, so no need to pass it from the client).

That would make us have to modify the response to something like:

{ "SSTablePreemptiveOpenInterval": 1234, "unit": "MB" }

What do you think?

We want to make endpoint generic, so underlying changes to C* shouldn't impact the endpoint usage for the end user, for example C* 6.0 returning value in different unit, while older versions continue to return the value in MB, the same endpoint should work regardless of the running c* version. Hence not including MB in the endpoint name. Whereas JMX operation name need to match exactly the JMX endpoint we are invoking. The same with passing option param like unit=MB, better we avoid supporting new unit param in the endpoint whenever C* changes for different versions.

In the response we need to somehow tell the end user that value returned is in in MB. I preferred the name SSTablePreemptiveOpenIntervalInMB as it matches the JMX operation currently the end users are calling. Your suggestion of separating the unit into another field sounds good, but that may complicate callers's code, as they need to parse unit as a string and convert value into those units accordingly. C* latest code changed this yaml param to for example "60MiB", let me see which one is simpler for the calling code "60MiB" or {60, "MiB"} and will get back to you

I did explore this a little, I believe the one you suggested /preemptive-open-interval?unit=mb is more easy way to implement and also is in-line with current JMX call semantics. Other alternatives unnecessarily complicates conversions from human readable format to byte size at the client.

Added unit=MiB param, unit names matches with names C* is using.

...java/org/apache/cassandra/sidecar/routes/cassandra/GetPreemptiveOpenIntervalHandlerTest.java

skoppu22 · 2024-12-11T15:54:21Z

...ase/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraStorageOperations.java

@@ -213,4 +216,17 @@ public void outOfRangeDataCleanup(@NotNull String keyspace, @NotNull String tabl
        jmxClient.proxy(StorageJmxOperations.class, STORAGE_SERVICE_OBJ_NAME)
                 .forceKeyspaceCleanup(concurrency, keyspace, table);
    }
+
+    @Override
+    public GetPreemptiveOpenIntervalResponse getSSTablePreemptiveOpenInterval(DataStorageUnit unit)


Removed 'inMB' from StorageOperations function names to make them generic for supporting other units later on.

skoppu22 · 2024-12-11T15:54:45Z

...ters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/StorageJmxOperations.java

+     * Invokes C* StorageServiceMBean's JMX function getSSTablePreemptiveOpenIntervalInMB
+     * @return the same value returned by the JMX operation
+     */
+    int getSSTablePreemptiveOpenIntervalInMB();


This is the only place we use 'inMB', this should exactly match C* JMX function name

skoppu22 · 2025-01-09T17:25:01Z

server/src/test/java/org/apache/cassandra/sidecar/routes/cassandra/JmxCommonTest.java

@@ -0,0 +1,120 @@
+/*


This file is same as in other PR https://github.com/apache/cassandra-sidecar/pull/159/files. Either one of these PRs will merge this file

bbotella

Looks good overall. Just a couple of nits. I'm +1 on this PR (nb)

bbotella · 2025-01-09T22:15:39Z

server/src/main/java/org/apache/cassandra/sidecar/routes/GetPreemptiveOpenIntervalHandler.java

+    protected DataStorageUnit extractParamsOrThrow(RoutingContext context)
+    {
+        // Only supported unit for preemptive open interval value is MB
+        String unit = RequestUtils.parseStringQueryParam(context.request(), "unit", "MiB");


Super nit: Maybe extracting the string MiB to a static DEFAULT_UNIT string type variable helps with readability.

bbotella · 2025-01-09T22:16:21Z

server/src/main/java/org/apache/cassandra/sidecar/utils/RequestUtils.java

@@ -61,4 +61,14 @@ public static Integer parseIntegerQueryParam(HttpServerRequest request, String p
        }
        return defaultValue;
    }
+
+    public static String parseStringQueryParam(HttpServerRequest request, String paramName, String defaultValue)


nit: You are technically getting the param, not parsing it. Consider renaming the method

bbotella · 2025-01-09T22:17:39Z

...ache/cassandra/sidecar/routes/cassandra/GetPreemptiveOpenIntervalHandlerIntegrationTest.java

+    @CassandraIntegrationTest(yamlProps = "sstable_preemptive_open_interval_in_mb=90")
+    void testPreemptiveOpenIntervalInvalidUnit(VertxTestContext testContext)
+    {
+        client.get(server.actualPort(), "127.0.0.1", testRoute + "?unit=KiB")


Nit: We may add support for KiB in the future, which would change the behavior of this test. Maybe just having a string that will never be a unit? Something like foo?

skoppu22 added 3 commits December 2, 2024 16:34

Sidecar endpoint to GET sstable's preemptive interval value

1aafa61

Integration test using dtest Sidecar client changes

Merge branch 'trunk' into jmxep1

e33ccad

default test case

de16561

skoppu22 force-pushed the jmxep1 branch from 26c7b04 to de16561 Compare December 3, 2024 16:00

bbotella reviewed Dec 3, 2024

View reviewed changes

skoppu22 added 2 commits December 5, 2024 13:18

Merge branch 'apache:trunk' into jmxep1

456caa4

remove jmx calls metrics

a27b56f

skoppu22 force-pushed the jmxep1 branch from 02f1ad8 to a27b56f Compare December 5, 2024 14:35

skoppu22 commented Dec 11, 2024

View reviewed changes

add optional param unit

9ae9912

skoppu22 force-pushed the jmxep1 branch from 191d9db to 9ae9912 Compare December 11, 2024 16:11

Merge branch 'trunk' into jmxep1

1c54822

skoppu22 commented Jan 9, 2025

View reviewed changes

skoppu22 force-pushed the jmxep1 branch from 70a841c to 195a51b Compare January 9, 2025 17:32

update with latest branch

f773b88

skoppu22 force-pushed the jmxep1 branch from 195a51b to f773b88 Compare January 9, 2025 17:34

bbotella reviewed Jan 9, 2025

View reviewed changes

bbotella approved these changes Jan 9, 2025

View reviewed changes

increase memory for tests

1cb1606

skoppu22 closed this Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSSIDECAR-162: Sidecar endpoint to GET sstable's preemptive interval value #152

CASSSIDECAR-162: Sidecar endpoint to GET sstable's preemptive interval value #152

skoppu22 commented Dec 3, 2024 •

edited

Loading

bbotella Dec 3, 2024

bbotella Dec 3, 2024

skoppu22 Dec 10, 2024

skoppu22 Jan 9, 2025

bbotella Dec 3, 2024

skoppu22 Dec 4, 2024

skoppu22 Dec 10, 2024

skoppu22 Dec 11, 2024

skoppu22 Dec 11, 2024

skoppu22 Dec 11, 2024

skoppu22 Jan 9, 2025

bbotella left a comment •

edited

Loading

bbotella Jan 9, 2025

bbotella Jan 9, 2025

bbotella Jan 9, 2025

CASSSIDECAR-162: Sidecar endpoint to GET sstable's preemptive interval value #152

CASSSIDECAR-162: Sidecar endpoint to GET sstable's preemptive interval value #152

Conversation

skoppu22 commented Dec 3, 2024 • edited Loading

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

bbotella left a comment • edited Loading

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

skoppu22 commented Dec 3, 2024 •

edited

Loading

bbotella left a comment •

edited

Loading