[
https://issues.apache.org/jira/browse/CASSANDRA-15234?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17125553#comment-17125553
]
Ekaterina Dimitrova edited comment on CASSANDRA-15234 at 6/4/20, 5:43 AM:
--------------------------------------------------------------------------
[ [Part 2
|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-3] ] [
[DTests
|https://github.com/ekaterinadimitrova2/cassandra-dtest/tree/CASSANDRA-15234-3]
] [ [CCM |https://github.com/ekaterinadimitrova2/ccm/tree/CASSANDRA-15234-3] ]
[ [CI run
|https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/187/workflows/615cdebf-1d49-47ab-93a4-efad25a247b1]
]
- Unit tests updated
- Getters and Setters names in the DatabaseDescriptor were updated to follow
the new names of the parameters updated
- The conversion of some of the parameters values which is happening in their
getters is kept as it is needed in runtime. Some parameters are used with
different units in the different areas of the codebase. So I left this part
untouched.
- New class created, ParseAndConvertUnits - it pulls all the functionality
needed to cover the addition of units suffixes to the Memory, Duration, and
Rate parameters in the cassandra.yaml
- the cassandra.yaml file in this branch contains currently the new parameters
with new names and unit suffixes but the arrangement of the sections is the old
one as this was causing me issues every time I rebase/commit. If we confirm the
format, this will be the last step to switch to the new format
- Added some additional parsing and supporting conversion functions for the
double duration parameters.
- The 2 failing cqlsh tests were fixed locally and tested. It was formatting
issue.
-1 unit test failed - it doesn't look a related issue but will check the config
loaded to ensure that tomorrow
- 8 failing dtest, it doesn't look related but I am gonna compare tomorrow
morning
- Current version of the code needs a final rebase/merge of the last 6 commits
on trunk. Will do tomorrow after I recheck the CI and current version
- Have to return the default requirements.txt and config.yml files. The current
versions were required only for testing purposes with CircleCI
*WARNING:* we should be careful with the dtests and ccm. One missed check for
old/new parameter and default value of the respective parameter from Config
will be loaded. This might not lead to failing test and thus silently cover the
miss.
*NOTE:* Custom Types, as they were suggested, don't really work with SnakeYAML,
unfortunately. SnakeYAML supports nothing more than mapping between the yaml
and the Config class.
* _So something like that: Memory max_value = new Memory("5ms") wouldn't work.
If there is no value in the yaml, the config default max_value will be parsed
properly but if there is value assigned to max_value at the yaml, it won't be
assigned properly in Config. Classes are used for nested parameters. Also,
currently there won't be a big benefit of the custom types as the different
parameters validations in the db descriptor are taking different approaches,
there is no general framework/approach._
_---------_
The last part was updated as per the recommendations. CI ran successfully. Need
to rebase and recheck nothing that might need the attention of this patch has
changed in the meanwhile. Three weeks passed since I took care of this one.
[ [Part3
|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-5] ] [
[CI JAVA8
|https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/146/workflows/fae5da98-acf5-4824-bf61-3ffab3d86cd6]
] [ [CI Java11
|https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/146/workflows/21b088c4-8984-4b20-8302-2ff8adaf55f7]
]
was (Author: e.dimitrova):
[ [Part 2
|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-3] ] [
[DTests
|https://github.com/ekaterinadimitrova2/cassandra-dtest/tree/CASSANDRA-15234-3]
] [ [CCM |https://github.com/ekaterinadimitrova2/ccm/tree/CASSANDRA-15234-3] ]
[ [CI run
|https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/187/workflows/615cdebf-1d49-47ab-93a4-efad25a247b1]
]
- Unit tests updated
- Getters and Setters names in the DatabaseDescriptor were updated to follow
the new names of the parameters updated
- The conversion of some of the parameters values which is happening in their
getters is kept as it is needed in runtime. Some parameters are used with
different units in the different areas of the codebase. So I left this part
untouched.
- New class created, ParseAndConvertUnits - it pulls all the functionality
needed to cover the addition of units suffixes to the Memory, Duration, and
Rate parameters in the cassandra.yaml
- the cassandra.yaml file in this branch contains currently the new parameters
with new names and unit suffixes but the arrangement of the sections is the old
one as this was causing me issues every time I rebase/commit. If we confirm the
format, this will be the last step to switch to the new format
- Added some additional parsing and supporting conversion functions for the
double duration parameters.
- The 2 failing cqlsh tests were fixed locally and tested. It was formatting
issue.
-1 unit test failed - it doesn't look a related issue but will check the config
loaded to ensure that tomorrow
- 8 failing dtest, it doesn't look related but I am gonna compare tomorrow
morning
- Current version of the code needs a final rebase/merge of the last 6 commits
on trunk. Will do tomorrow after I recheck the CI and current version
- Have to return the default requirements.txt and config.yml files. The current
versions were required only for testing purposes with CircleCI
*WARNING:* we should be careful with the dtests and ccm. One missed check for
old/new parameter and default value of the respective parameter from Config
will be loaded. This might not lead to failing test and thus silently cover the
miss.
*NOTE:* Custom Types, as they were suggested, don't really work with SnakeYAML,
unfortunately. SnakeYAML supports nothing more than mapping between the yaml
and the Config class.
* _So something like that: Memory max_value = new Memory("5ms") wouldn't work.
If there is no value in the yaml, the config default max_value will be parsed
properly but if there is value assigned to max_value at the yaml, it won't be
assigned properly in Config. Classes are used for nested parameters. Also,
currently there won't be a big benefit of the custom types as the different
parameters validations in the db descriptor are taking different approaches,
there is no general framework/approach._
_---------_
The last part was updated as per the recommendations. CI ran successfully. Need
to rebase and recheck nothing that might need the attention of this patch has
changed in the meanwhile. Three weeks passed since I took care of this one.
[ [Part3
|https://github.com/ekaterinadimitrova2/cassandra/tree/CASSANDRA-15234-5] ] [
[CI JAVA8
|https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/146/workflows/fae5da98-acf5-4824-bf61-3ffab3d86cd6]
] [ [CI Java11
|https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/146/workflows/21b088c4-8984-4b20-8302-2ff8adaf55f7]
]
> 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-alpha
>
>
> 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
