[
https://issues.apache.org/jira/browse/CASSANDRA-15234?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17086118#comment-17086118
]
Ekaterina Dimitrova edited comment on CASSANDRA-15234 at 4/17/20, 10:44 PM:
----------------------------------------------------------------------------
Done but with one caveat which was caught thankfully with the DTests. Unit
tests were even on 100% green at some runs. (And my unit test was not covering
this conversion, I should add it!)
I am new to snakeyaml so I might be wrong, I would appreciate if someone who
has the experience proves me wrong.
>From the DTest failures I realized that turning into Strings the values from
>the YAML we hit one issue. To me it looks like we already have it actually
>with Integer and String parameters, just they are not many so it didn't byte
>us up to now. Not sure whether this was considered when the Config class was
>built or not.
So the issue with int parameters in Config in my solution is:
Currently in trunk, when we parse to int variable, If a parameter is commented
in the cassandra.yaml, this is observed as null and it will raise an exception
in some cases. (i.e. commitlog_sync_period_in_ms) But if it is not commented,
just not assigned a value, then this is considered as 0 (as we parse to int).
The situation is different with String, Integer. They will be in both cases
null. No differentiator. But we don't have many parameters being Integer and
String and it didn't byte us.
Now with the units attached to the parameters in the cassandra.yaml file, we
need to parse strings with snakeyaml and then convert to the proper int values
in Config.
A way to cope with this issue is to make the users use "". In this way, if we
have a parameter not commented, but not also having an assigned value in
cassandra.yaml, we can identify it by checking a string for being empty. If the
parameter is commented or not presented, it will be null. But this would be one
more new thing for the users to consider. That's why I came back to snakeyaml
again and tried to think of a different way utilizing it. Unfortunately, up to
now I didn't find any different solution. Also, the links to the examples in
the official snakeyaml documentation are broken. Not sure whether there they
had some interesting stuff. Maybe something with customized constructor but
also, I try to make as less as possible disruptive changes in the codebase in
order not to mess up something with the Config which might be extremely
unpleasant.
I will be happy to hear an experienced opinion here. Does anyone know a better
solution? Or are we ok to introduce "" in the yaml for empty
parameters/Strings recognition?
[Part1|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-2]
(This part was already revised by [~dcapwell] but I paste the explanation again
here for completeness).
- New cassandra.yaml ---> currently cassandra-new.yaml in the branch as I
wanted to test the backward compatibility on the old version (will have to
rename properly also these yaml files with version probably)
reorganization of the file renames to make the names more consistent
- Backward compatibility with the old names - my approach is:
Cassandra comes with the new version of the yaml to ensure any new builds will
be using it
During load of the yaml file I check whether old names are identified(means
someone upgraded to 4 and wants to still use the old yaml). If this is the
case, we load the values from the old yaml to the new variables (the already
renamed parameters). Also, there is a warning to the user that there is new
cassandra.yaml version and he/she can consider an update.
[Part2|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-3]
- units added to the values in the cassandra.yaml file. Parsing and unit
conversion in place. One unit test added, should be further developed.
[DTests|https://github.com/ekaterinadimitrova2/cassandra-dtest] updated
accordingly for v.4
[Part3|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-5]
- SysProperties class, accessors, hope I didn't miss anything... most of them
are actually just using
TO DO: Extract the parser/converter in a separate class from Config
[~mck]? [~dcapwell]? [~benedict]? Anyone else any thoughts here?
NOTE: The branches are not fully cleared, this is still work in progress
was (Author: e.dimitrova):
Done but with one caveat which was caught thankfully with the DTests. Unit
tests were even on 100% green at some runs. (And my unit test was not covering
this conversion, I should add it!)
I am new to snakeyaml so I might be wrong, I would appreciate if someone who
has the experience proves me wrong.
>From the DTest failures I realized that turning into Strings the values from
>the YAML we hit one issue. To me it looks like we already have it actually
>with Integer and String parameters, just they are not many so it didn't byte
>us up to now. Not sure whether this was considered when the Config class was
>built or not.
So the issue with int parameters in Config in my solution is:
Currently in trunk, when we parse to int variable, If a parameter is commented
in the cassandra.yaml, this is observed as null and it will raise an exception
in some cases. (i.e. commitlog_sync_period_in_ms) But if it is not commented,
just not assigned a value, then this is considered as 0 (as we parse to int).
The situation is different with String, Integer. They will be in both cases
null. No differentiator. But we don't have many parameters being Integer and
String and it didn't byte us.
Now with the units attached to the parameters in the cassandra.yaml file, we
need to parse strings with snakeyaml and then convert to the proper int values
in Config.
A way to cope with this issue is to make the users use "". In this way, if we
have a parameter not commented, but not also having an assigned value in
cassandra.yaml, we can identify it by checking a string for being empty. If the
parameter is commented or not presented, it will be null. But this would be one
more new thing for the users to consider. That's why I came back to snakeyaml
again and tried to think of a different way utilizing it. Unfortunately, up to
now I didn't find any different solution. Also, the links to the examples in
the official snakeyaml documentation are broken. Not sure whether there they
had some interesting stuff. Maybe something with customized constructor but
also, I try to make as less as possible disruptive changes in the codebase in
order not to mess up something with the Config which might be extremely
unpleasant.
I will be happy to hear an experienced opinion here. Does anyone know a better
solution? Or are we ok to introduce "" in the yaml for empty
parameters/Strings recognition?
[Part1|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-2]
(This part was already revised by [~dcapwell]David but I paste the explanation
again here for completeness).
- New cassandra.yaml ---> currently cassandra-new.yaml in the branch as I
wanted to test the backward compatibility on the old version (will have to
rename properly also these yaml files with version probably)
reorganization of the file renames to make the names more consistent
- Backward compatibility with the old names - my approach is:
Cassandra comes with the new version of the yaml to ensure any new builds will
be using it
During load of the yaml file I check whether old names are identified(means
someone upgraded to 4 and wants to still use the old yaml). If this is the
case, we load the values from the old yaml to the new variables (the already
renamed parameters). Also, there is a warning to the user that there is new
cassandra.yaml version and he/she can consider an update.
[Part2|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-3]
- units added to the values in the cassandra.yaml file. Parsing and unit
conversion in place. One unit test added, should be further developed.
[DTests|https://github.com/ekaterinadimitrova2/cassandra-dtest] updated
accordingly for v.4
[Part3|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-5]
- SysProperties class, accessors, hope I didn't miss anything... most of them
are actually just using
TO DO: Extract the parser/converter in a separate class from Config
[~mck]? [~dcapwell]? [~benedict]? Anyone else any thoughts here?
NOTE: The branches are not fully cleared, this is still work in progress
> Standardise config and JVM parameters
> -------------------------------------
>
> Key: CASSANDRA-15234
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15234
> Project: Cassandra
> Issue Type: Bug
> Components: Local/Config
> Reporter: Benedict Elliott Smith
> Assignee: Ekaterina Dimitrova
> Priority: Normal
> Fix For: 4.0, 4.0-beta
>
>
> We have a bunch of inconsistent names and config patterns in the codebase,
> both from the yams and JVM properties. It would be nice to standardise the
> naming (such as otc_ vs internode_) as well as the provision of values with
> units - while maintaining perpetual backwards compatibility with the old
> parameter names, of course.
> For temporal units, I would propose parsing strings with suffixes of:
> {{code}}
> u|micros(econds?)?
> ms|millis(econds?)?
> s(econds?)?
> m(inutes?)?
> h(ours?)?
> d(ays?)?
> mo(nths?)?
> {{code}}
> For rate units, I would propose parsing any of the standard {{B/s, KiB/s,
> MiB/s, GiB/s, TiB/s}}.
> Perhaps for avoiding ambiguity we could not accept bauds {{bs, Mbps}} or
> powers of 1000 such as {{KB/s}}, given these are regularly used for either
> their old or new definition e.g. {{KiB/s}}, or we could support them and
> simply log the value in bytes/s.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
