[DISCUSSION] Cassandra-17515 - course of action

[Ekaterina Dimitrova <e....@gmail.com>]

Hi everyone,
While working on CASSANDRA-17431 to move the advanced Config parameters we
don't advertise in cassandra.yaml to the new types created in
CASSANDRA-15234, we ran into some concerns around
native_transport_max_concurrent_requests_in_bytes
and native_transport_max_concurrent_requests_in_bytes_per_ip.
While on startup if those parameters are set to something <= 0, they will
get a default value, their setters are not guarded in the same way, we just
set any long value.
I checked and this is actually something going on all currently supported
Cassandra versions. From there Caleb raised the question that 0 actually
can be a valid value and the check needs to cover possibly only < 0.
As these are advanced and non-documented parameters, I thought it is
valuable to confirm the course of action here. Especially, if we are
willing to do a change to all released versions.
So the proposal is to:
1) change the check on startup from <=0 to <0 and make 0 a valid value, for
all versions?
2) Add the same checks&assignment to the respective setters
3) Update NEWS.txt explicitly (this part is a bit weird to me as we keep it
not advertised in cassandra.yaml but we want whoever uses it to know about
such changes...)

Looking forward to your feedback, thanks in advance.

Best regards,
Ekaterina

Re: [DISCUSSION] Cassandra-17515 - course of action

[Ekaterina Dimitrova <e....@gmail.com>]

So my suggestion from what I hear
 - reject anything below -1.
- -1 points to the default.
- >=0 being valid.
- Done for both startup and the setters.
-  Done only on trunk for now and I will add comment in Config instead of
NEWS.txt due to their hidden nature.
So my understanding is that 0 and 1 will do kind of the same but it is just
not right not to accept 0 and point it to the default value which -1 does.
Please correct me if I am wrong.
Also, we anyway reject negatives in trunk with the new types except for
some special cases where things like -1 were legit before for some
parameters.  (More info in the Converters) Those will still work with the
old yaml and being translated to null with the new types. Documented

On Mon, 4 Apr 2022 at 16:54, David Capwell <dc...@apple.com> wrote:

> I am good with making >= 0 valid in both setters and config parsing,
> negatives should error or produce defaults
>
>
> On Apr 4, 2022, at 10:45 AM, Caleb Rackliffe <ca...@gmail.com>
> wrote:
>
> I'm for making >= 0 valid in both the setters and on startup. In the
> setters, I'm fine with either translating negative values to the default
> calculated value based on heap size or simply rejecting negative values. If
> we really want to override that value, we had better have a really good
> idea of why...
>
> Given this has always been an advanced feature, I'm neutral on adding
> anything to NEWS.txt.
>
> On Mon, Apr 4, 2022 at 8:46 AM Ekaterina Dimitrova <e....@gmail.com>
> wrote:
>
>> Hi everyone,
>> While working on CASSANDRA-17431 to move the advanced Config parameters
>> we don't advertise in cassandra.yaml to the new types created in
>> CASSANDRA-15234, we ran into some concerns around native_transport_max_concurrent_requests_in_bytes
>> and native_transport_max_concurrent_requests_in_bytes_per_ip.
>> While on startup if those parameters are set to something <= 0, they will
>> get a default value, their setters are not guarded in the same way, we just
>> set any long value.
>> I checked and this is actually something going on all currently supported
>> Cassandra versions. From there Caleb raised the question that 0 actually
>> can be a valid value and the check needs to cover possibly only < 0.
>> As these are advanced and non-documented parameters, I thought it is
>> valuable to confirm the course of action here. Especially, if we are
>> willing to do a change to all released versions.
>> So the proposal is to:
>> 1) change the check on startup from <=0 to <0 and make 0 a valid value,
>> for all versions?
>> 2) Add the same checks&assignment to the respective setters
>> 3) Update NEWS.txt explicitly (this part is a bit weird to me as we keep
>> it not advertised in cassandra.yaml but we want whoever uses it to know
>> about such changes...)
>>
>> Looking forward to your feedback, thanks in advance.
>>
>> Best regards,
>> Ekaterina
>>
>
>

Re: [DISCUSSION] Cassandra-17515 - course of action

[David Capwell <dc...@apple.com>]

I am good with making >= 0 valid in both setters and config parsing, negatives should error or produce defaults

> On Apr 4, 2022, at 10:45 AM, Caleb Rackliffe <ca...@gmail.com> wrote:
> 
> I'm for making >= 0 valid in both the setters and on startup. In the setters, I'm fine with either translating negative values to the default calculated value based on heap size or simply rejecting negative values. If we really want to override that value, we had better have a really good idea of why...
> 
> Given this has always been an advanced feature, I'm neutral on adding anything to NEWS.txt.
> 
> On Mon, Apr 4, 2022 at 8:46 AM Ekaterina Dimitrova <e.dimitrova@gmail.com <ma...@gmail.com>> wrote:
> Hi everyone,
> While working on CASSANDRA-17431 to move the advanced Config parameters we don't advertise in cassandra.yaml to the new types created in CASSANDRA-15234, we ran into some concerns around native_transport_max_concurrent_requests_in_bytes and native_transport_max_concurrent_requests_in_bytes_per_ip.
> While on startup if those parameters are set to something <= 0, they will get a default value, their setters are not guarded in the same way, we just set any long value. 
> I checked and this is actually something going on all currently supported Cassandra versions. From there Caleb raised the question that 0 actually can be a valid value and the check needs to cover possibly only < 0.
> As these are advanced and non-documented parameters, I thought it is valuable to confirm the course of action here. Especially, if we are willing to do a change to all released versions. 
> So the proposal is to:
> 1) change the check on startup from <=0 to <0 and make 0 a valid value, for all versions?
> 2) Add the same checks&assignment to the respective setters
> 3) Update NEWS.txt explicitly (this part is a bit weird to me as we keep it not advertised in cassandra.yaml but we want whoever uses it to know about such changes...)
> 
> Looking forward to your feedback, thanks in advance.
> 
> Best regards,
> Ekaterina

Re: [DISCUSSION] Cassandra-17515 - course of action

[Caleb Rackliffe <ca...@gmail.com>]

I'm for making >= 0 valid in both the setters and on startup. In the
setters, I'm fine with either translating negative values to the default
calculated value based on heap size or simply rejecting negative values. If
we really want to override that value, we had better have a really good
idea of why...

Given this has always been an advanced feature, I'm neutral on adding
anything to NEWS.txt.

On Mon, Apr 4, 2022 at 8:46 AM Ekaterina Dimitrova <e....@gmail.com>
wrote:

> Hi everyone,
> While working on CASSANDRA-17431 to move the advanced Config parameters we
> don't advertise in cassandra.yaml to the new types created in
> CASSANDRA-15234, we ran into some concerns around native_transport_max_concurrent_requests_in_bytes
> and native_transport_max_concurrent_requests_in_bytes_per_ip.
> While on startup if those parameters are set to something <= 0, they will
> get a default value, their setters are not guarded in the same way, we just
> set any long value.
> I checked and this is actually something going on all currently supported
> Cassandra versions. From there Caleb raised the question that 0 actually
> can be a valid value and the check needs to cover possibly only < 0.
> As these are advanced and non-documented parameters, I thought it is
> valuable to confirm the course of action here. Especially, if we are
> willing to do a change to all released versions.
> So the proposal is to:
> 1) change the check on startup from <=0 to <0 and make 0 a valid value,
> for all versions?
> 2) Add the same checks&assignment to the respective setters
> 3) Update NEWS.txt explicitly (this part is a bit weird to me as we keep
> it not advertised in cassandra.yaml but we want whoever uses it to know
> about such changes...)
>
> Looking forward to your feedback, thanks in advance.
>
> Best regards,
> Ekaterina
>