[
https://issues.apache.org/jira/browse/CASSANDRA-15234?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17162209#comment-17162209
]
Ekaterina Dimitrova edited comment on CASSANDRA-15234 at 7/21/20, 5:53 PM:
---------------------------------------------------------------------------
Good catch [~benedict] . Thank you. My bad, just pushed a fast commit
[here|https://github.com/ekaterinadimitrova2/cassandra/commit/b4eebe080835da79d032f9314262c268b71172a8]
for the three rate parameters we have.
For the record, I stopped working on this patch until it is clear whether the
code will be used at all. As far as I understand, you took the lead on a POC
development for the newly suggested approach. Unfortunately, I don't have time
to support this now but I would be happy to give feedback when it is shaped
already.
If you decide at some point this work or part of it to be committed, let me
know, I will complete whatever is outstanding. The main framework is in place.
A couple of things to keep in mind:
* As suggested by Benjamin, the patch can be simplified by making
{{Converter}} an {{enum}}. It will allow to use the enum value directly into
the {{Replaces}} annotation. Doing so will remove the need to use reflection to
instantiate the converters and the use of a cache to avoid multiple
instantiation.
* In-jvm tests - loading config parameters should be reworked as currently
they don't work with custom types (like the newly introduced Duration, etc).
The suggested approach by [~dcapwell] would work but it requires also api
changes. I suggest a separate ticket for this part to be opened. Instead of
using reflection, the suggestion is to use SnakeYAML. In order not to slow down
the tests, no yaml files will be introduced but there will be a function to
build the yaml nodes for us. This was a quick POC by [~dcapwell] but there are
parameters which will need additional work and attention:
{code:java}
public class MapToConfigTest
{
@Test
public void test()
{
Map<String, Object> map = ImmutableMap.<String, Object>builder()
.put("auto_bootstrap", false)
.put("permissions_validity_in_ms", 10)
.put("role_manager",
"some value")
.build(); Constructor
constructor = new YamlConfigurationLoader.CustomConstructor(Config.class);
PropertiesChecker propertiesChecker = new PropertiesChecker();
constructor.setPropertyUtils(propertiesChecker);
constructor.setComposer(new Composer(null, null)
{
public Node getSingleNode()
{
return toNode(map);
}
});
Config config = (Config) constructor.getSingleData(Config.class);
System.out.println("trap");
}
public static Node toNode(Object object)
{
if (object instanceof Map)
{
List<NodeTuple> values = new ArrayList<>();
for (Map.Entry<Object, Object> e : ((Map<Object, Object>)
object).entrySet())
{
values.add(new NodeTuple(toNode(e.getKey()),
toNode(e.getValue())));
}
return new MappingNode(FAKE_TAG, values, null);
}
else if (object instanceof Number || object instanceof String ||
object instanceof Boolean)
{
return new ScalarNode(FAKE_TAG, object.toString(), FAKE_MARK,
FAKE_MARK, '\'');
}
else
{
throw new UnsupportedOperationException("unexpected type found:
given " + object.getClass());
}
}
private static final Tag FAKE_TAG = new Tag("ignore");
private static final Mark FAKE_MARK = new Mark("ignore", 0, 0, 0, "", 0);
}
{code}
The rest are small things which I also partially already handled.
[~maedhroz], [~benedict], please, let me know if you have any other questions
around this patch.
was (Author: e.dimitrova):
Good catch [~benedict] . Thank you. My bad, just pushed a fast commit
[here|https://github.com/ekaterinadimitrova2/cassandra/commit/b4eebe080835da79d032f9314262c268b71172a8]
for the three rate parameters we have.
For the record, I stopped working on this patch until it is clear whether the
code will be used at all. As far as I understand, you took the lead on a POC
development for the newly suggested approach. Unfortunately, I don't have time
to support this now but I would be happy to give feedback when it is shaped
already.
If you decide at some point this work or part of it to be committed, let me
know, I will complete whatever is outstanding. The main framework is in place.
A couple of things to keep in mind:
* As suggested by Benjamin, the patch can be simplified by making
{{Converter}} an {{enum}}. It will allow to use the enum value directly into
the {{Replaces}} annotation. Doing so will remove the need to use reflection to
instantiate the converters and the use of a cache to avoid multiple
instantiation.
* In-jvm tests - loading config parameters should be reworked as currently
they don't work with custom types (like the newly introduced Duration, etc).
The suggested approach by [~dcapwell] would work but it requires also api
changes. I suggest a separate ticket for this part to be opened. Instead of
using reflection, the suggestion is to use SnakeYAML. In order not to slow down
the tests, no yaml files will be introduced but there will be a function to
build the yaml nodes for us. This was a quick POC by [~dcapwell] but there are
parameters which will need additional work and attention:
public class MapToConfigTest
{
@Test
public void test()
{
Map<String, Object> map = ImmutableMap.<String, Object>builder()
.put("auto_bootstrap", false)
.put("permissions_validity_in_ms", 10)
.put("role_manager", "some value")
.build(); Constructor constructor =
new YamlConfigurationLoader.CustomConstructor(Config.class);
PropertiesChecker propertiesChecker = new PropertiesChecker();
constructor.setPropertyUtils(propertiesChecker);
constructor.setComposer(new Composer(null, null) {
public Node getSingleNode()
{
return toNode(map);
}
}); Config config = (Config)
constructor.getSingleData(Config.class);
System.out.println("trap");
} public static Node toNode(Object object)
{
if (object instanceof Map)
{
List<NodeTuple> values = new ArrayList<>();
for (Map.Entry<Object, Object> e : ((Map<Object, Object>)
object).entrySet())
{
values.add(new NodeTuple(toNode(e.getKey()),
toNode(e.getValue())));
}
return new MappingNode(FAKE_TAG, values, null);
}
else if (object instanceof Number || object instanceof String || object
instanceof Boolean)
{
return new ScalarNode(FAKE_TAG, object.toString(), FAKE_MARK,
FAKE_MARK, '\'');
}
else
{
throw new UnsupportedOperationException("unexpected type found:
given " + object.getClass());
}
} private static final Tag FAKE_TAG = new Tag("ignore");
private static final Mark FAKE_MARK = new Mark("ignore", 0, 0, 0, "", 0);
}
The rest are small things which I also partially already handled.
[~maedhroz], [~benedict], please, let me know if you have any other questions
around this patch.
> 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-beta
>
> Attachments: CASSANDRA-15234-3-DTests-JAVA8.txt
>
>
> 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
