Verifies that the CassandraProperties default values are the same as driver built-in defaults

[onobc]

Fixes gh-24965

Points Of Interest

Developer documentation

I was not sure the best way to document "Hey if you set a default value and it aligns w/ the driver default add a test for it in assandraPropertiesTest". For now I put an inline comment in the CasssandraProperties.

Generally align w/ defaults

I am assuming that we want to generally align w/ the driver defaults.

We have a few properties we set defaults for that have no default in the driver. The following tests would fail as the default value in the driver is null. I omitted these tests:

assertThat(properties.getRequest().getThrottler().getMaxQueueSize()).isEqualTo(
        optionsMap.get(TypedDriverOption.REQUEST_THROTTLER_MAX_QUEUE_SIZE));

assertThat(properties.getRequest().getThrottler().getMaxConcurrentRequests()).isEqualTo(
        optionsMap.get(TypedDriverOption.REQUEST_THROTTLER_MAX_CONCURRENT_REQUESTS));

assertThat(properties.getRequest().getThrottler().getMaxRequestsPerSecond()).isEqualTo(
        optionsMap.get(TypedDriverOption.REQUEST_THROTTLER_MAX_REQUESTS_PER_SECOND));

assertThat(properties.getRequest().getThrottler().getDrainInterval()).isEqualTo(
        optionsMap.get(TypedDriverOption.REQUEST_THROTTLER_DRAIN_INTERVAL));

Also, we have a property that is divergent from the driver (test omitted as well):

// This breaks as we have 120s and the driver has 500ms
assertThat(properties.getPool().getIdleTimeout()).isEqualTo(
                optionsMap.get(TypedDriverOption.HEARTBEAT_TIMEOUT));

[snicoll]

Thanks for the PR Chris. I've added a few comments, can you please take a look?

[snicoll]

We don't need this. Whenever the team adds a property with a default value, they look for a test that checks default values and add an assertion.

[onobc]

Cool. Yeh, I wanted to put thoughts there and then get guidance from team. I will remove.

[snicoll]

We don't add comments in code like that. Please revert.

[onobc]

Same as above.

[snicoll]

While I can't deny it makes the assertions nicer, we're not doing there anywhere else. If you want to discuss the merit of adding such a message, I am happy to consider it but it should be part of a separate issue.

[onobc]

This would only be valuable in an ecosystem where the team was not aware of defaults and what that all entails (adding an entry to this test, etc). I would definitely add this in my codebase at work but as you stated above, its not needed here. Everyone is "in the know". Removing..

[onobc]

@snicoll thanks for the feedback. I have made the suggested changes.

Also, can you comment on the 2 points I added in the PR description?

1. We have a few properties we set defaults for that have no default in the driver.

2. We have a property that is divergent from the driver.

[snicoll]

Also, can you comment on the 2 points I added in the PR description?

Sorry, I missed those.

We have a few properties we set defaults for that have no default in the driver.

When we put a default back then, I based it on reference.conf which can be a bit hard to read as some defaults are based on the default value of another property.

Taking the first property as an example, it is 10000 in our metadata and the reference.conf of the driver has the same value. It looks commented out though so I am not so sure I made the right call documenting those as such.

@adutra can you help us out?

We have a property that is divergent from the driver.

Either a mistake, or the default change in between which is exactly what this PR is supposed to handle.

[onobc]

Either a mistake, or the default change in between which is exactly what this PR is supposed to handle.

@snicoll I will create a follow on PR that puts the values in parity and adds the test to verify. Or do you prefer that we do it in this PR?

[snicoll]

@Bono007 thanks for asking. This one is a task to improve our test suite and avoid a regression or an inconsistency. If a default for a property is invalid, this is a bug and we'd like another issue just that it shows up in the appropriate place in the release notes.

Let's finalize this one and see what @adutra thinks about the current state of affairs.

[adutra]

Hello!

First of all, let's keep in mind that there is no finite set of "default values" in the driver because you can replace virtually any of its components with your own, but 3rd-party components may require different settings to be properly configured. Sometimes too, the driver itself ships with 2 or 3 alternatives of the same component, each of them having its own set of settings. This is why many settings appear commented out in reference.conf. I understand that this doesn't make it any easier to grasp which settings you need to define and when, but that was the price to pay to attain this level of customizability.

The closest thing to that notion would be the notion of all the settings that the driver reads, along with their values, when nothing is customized. This is reflected by OptionsMap#driverDefaults indeed. But this also means that a setting that is commented out in reference.conf won't appear in that map either.

In the case of heartbeat timeouts, the problem is again JAVA-2841: a few timeouts were raised to 5 seconds by default. Just fix it by setting it to 5 seconds in Spring Boot and we should be good. I promise that we definitely do not want to touch these things again (we did it because of cloud deployments).

In the case of request throttlers, there is indeed no default value for settings such as REQUEST_THROTTLER_MAX_QUEUE_SIZE because the default throttler is a no-op throttler that doesn't need this. The commented-out value that you see in reference.conf is really just a suggestion, and imo depends a lot on the use case at hand. ideally, I'd argue that Spring Boot shouldn't be defining a setting that the driver won't necessarily need, but I understand the constraints. If you absolutely must provide a default value for it, then go for 10000 but maybe you should add a note to the javadocs encouraging users to benchmark this value until they come up with the best one for their case.

Let me know if I can help with anything else. And thanks for putting up this PR!

[onobc]

Thank you @adutra for the detail and the links - it was really helpful.

@snicoll

A couple of observations:

• In 2.4.x the Cassandra driver version is 4.9 and has the new defaults.
• In 2.3.x the Cassandra driver version is 4.6 and it does not have the new defaults (they were added in 4.8.1)
• The motivating issue for this PR is 2.3.x.

Here is what I am thinking:

1. Use this PR in 2.3.x with the current default checks and no changes to defaults - it does reflect reality in the 4.6 driver.
2. Create an issue in 2.4 w/ the following details:

• Decide to remove or keep the throttler defaults with appropriate docs created/updated based on keep/removal
  • TypedDriverOption.REQUEST_THROTTLER_MAX_QUEUE_SIZE));
  • TypedDriverOption.REQUEST_THROTTLER_MAX_CONCURRENT_REQUESTS));
  • TypedDriverOption.REQUEST_THROTTLER_MAX_REQUESTS_PER_SECOND));
  • TypedDriverOption.REQUEST_THROTTLER_DRAIN_INTERVAL));
• Adjust the following default timeout values

  spring.data.cassandra.pool.idle-timeout: 5s
  spring.data.cassandra.pool.heartbeat-interval: 5s
  

• Update the default versions test to cover the above cases

Once we land on direction I am happy to continue this effort and implement those changes.

[snicoll]

ideally, I'd argue that Spring Boot shouldn't be defining a setting that the driver won't necessarily need, but I understand the constraints. If you absolutely must provide a default value for it, then go for 10000 but maybe you should add a note to the javadocs encouraging users to benchmark this value until they come up with the best one for their case.

No, we don't need to provide a value but we offer a higher-view on them and the default should be consistent IMO. Right now, the default for the type is NONE which shows up in the metadata to let the user know that we don't use any sort of throttling by default. That's good, we want that.

Let's consider the use case of a user enabling throttling using that property to, let's say, "rate limiting". I'd expect that we have sensible defaults for the queue size and such. If we don't provide a default value for this, then the queue size is 0 which looks like it's going to reject requests almost immediately. Am I right?

What I am trying to show here is that using one property enables a behaviour that should work OOB (or fail because a mandatory property isn't set).

[snicoll]

@Bono007 let's go ahead with what we can test (i.e. a default that is effectively available in OptionsMap#driverDefaults. We can have a separate issue for the default that's inconsistent in 2.4.xthat we can fix prior to merging this one.

I am not sure about the 4 throttling options yet.

[onobc]

@snicoll sounds good.

let's go ahead with what we can test (i.e. a default that is effectively available in OptionsMap#driverDefaults

I think we have that here in this PR - only exception is the discrepancy between default for HEARTBEAT_TIMEOUT (we have 120s and the 4.6 driver has 500ms).

[snicoll]

only exception is the discrepancy between default for HEARTBEAT_TIMEOUT (we have 120s and the 4.6 driver has 500ms).

Can you raise another PR against 2.4.x that fixes that? If so I can merge it first and then I'll take care of this one. If not, just let me know and I'll go ahead.

[adutra]

Let's consider the use case of a user enabling throttling using that property to, let's say, "rate limiting". I'd expect that we have sensible defaults for the queue size and such. If we don't provide a default value for this, then the queue size is 0 which looks like it's going to reject requests almost immediately. Am I right?

Yes, a queue size of zero would mean queries are rejected as soon as all available permits are exhausted. Definitely not a good default value.

[snicoll]

@adutra thanks. So where do we go from here? Isn't a regular user of Cassandra also affected by this? They would set the property to switch the throttling type and end up with the same behaviour as far as I can see.

[adutra]

@adutra thanks. So where do we go from here? Isn't a regular user of Cassandra also affected by this? They would set the property to switch the throttling type and end up with the same behaviour as far as I can see.

Hm sorry we might have a misunderstanding here.

My previous comment was referring to the hypothesis where Spring Boot would explicitly define a default value of zero for datastax-java-driver.advanced.throttler.max-queue-size. Combined with the property that enables rate limiting, this would lead to the undesirable situation where, if a user enables rate limiting but forgets to change the default queue size, the driver would happily enable rate limiting as requested, but with a max queue size of zero.

A regular driver user, on the other hand, would get an error. If they define datastax-java-driver.advanced.throttler.class = RateLimitingRequestThrottler but forget to also define some value for datastax-java-driver.advanced.throttler.max-queue-size (since there is no default value for that in reference.conf), then the driver would complain and fail to initialize itself.

In short: can you set max queue size to some special value, maybe Optional.empty()? That would be the best solution imo. In any case, setting it to zero would be a bad idea.

Sorry I hope I've made myself more clear this time.

[onobc]

Can you raise another PR against 2.4.x that fixes that? If so I can merge it first and then I'll take care of this one. If not, just let me know and I'll go ahead.

Sure. Doing that now. Should be in ~30mins

[snicoll]

No, we would never do that of course, quoting myself

If we don't provide a default value for this

Alright so failure is the other case I was referring to in this conversation and that's good the driver behaves that way. We don't need Optional.empty. We have a bunch of properties that don't have a default value and we just don't do anything for those if the user hasn't set them. I've created #25149, thanks @adutra!

[snicoll]

Thanks a lot @Bono007 for the follow-up!