[
https://issues.apache.org/jira/browse/METRON-1336?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16273644#comment-16273644
]
ASF GitHub Bot commented on METRON-1336:
----------------------------------------
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154230654
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -343,25 +343,57 @@ public static void uploadConfigsToZookeeper(String
rootFilePath, CuratorFramewor
* @param type config type to upload configs for
* @param configName specific config under the specified config type
*/
- public static void uploadConfigsToZookeeper(String rootFilePath,
CuratorFramework client,
- ConfigurationType type, Optional<String> configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional<String> configName) throws Exception {
+
switch (type) {
+
case GLOBAL:
final byte[] globalConfig = readGlobalConfigFromFile(rootFilePath);
if (globalConfig.length > 0) {
setupStellarStatically(client, Optional.of(new
String(globalConfig)));
writeGlobalConfigToZookeeper(globalConfig, client);
}
break;
- case PARSER: // intentional pass-through
- case ENRICHMENT: // intentional pass-through
- case INDEXING:
- Map<String, byte[]> sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
- configName);
- for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+ Map<String, byte[]> configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
I remember struggling through this class when applying the patching
mechanism, so I want to start by saying I'm a big fan of this work and
conversation around how to best improve and simplify this class. @cestella I
agree with @nickwallen about option 1. Compared to your PR on this PR (sounds
like an XZibit meme...) for option 3, I think it's much less intuitive to read.
I took a look through it and that would be my preference for the way to go.
It's classic strategy pattern through composition, and it eliminates the
duplication we see below. I tend to agree with the comments on that PR about
eventually pulling those implementations into their own classes. That would
make the code much more readable. And I think it probably puts us in a better
place for cleaning up the config utils class or removing/renaming it
altogether. The only part of this that is still a little bit of a smell to me
is that the global config and profiler throw unsupported operation exceptions.
I think this is a big enough improvement that I'd be willing to let that slide
for now, but ideally the global config would exist similarly to the other
config. e.g. instead of this:
- config/zookeeper/
- global.json
- profiler.json
- parsers/
- ...
- enrichment/
- ...
- indexing/
- ...
- .... etc.
You'd give the global config and profiler a directory as well, which would
enable a consistent layout on the local FS and in Zookeeper, and a consistent
interface for these commands.
- config/zookeeper/
- global/
- global.json
- profiler/
- profiler.json
- parsers/
- ...
Then this
```
public interface ConfigurationOperations {
...
void writeSensorConfigToZookeeper(String sensorType, byte[] configData,
CuratorFramework client) throws Exception;
...
```
could become this
```
public interface ConfigurationOperations {
...
void writeConfigToZookeeper(String type, byte[] configData,
CuratorFramework client) throws Exception;
```
I'm not sure it's desirable or not, but you could then theoretically
provide multiple configs in global/ and profiler/, which is consistent with the
parser/, enrichment/, and indexing/ configs.
> Patching Can Result in Bad Configuration
> ----------------------------------------
>
> Key: METRON-1336
> URL: https://issues.apache.org/jira/browse/METRON-1336
> Project: Metron
> Issue Type: Bug
> Reporter: Nick Allen
> Assignee: Nick Allen
> Fix For: 0.4.1
>
>
> When applying a patch with `zk_load_configs` the resulting configuration can
> be invalid. The resulting configuration should be validated so that a patch
> can never result in an invalid configuration.
> For example, applying the following patch with `zk_load_config` to the
> Profiler yields a broken Profiler configuration.
> {code}
> [
> {
> "path":"profiles",
> "value":{
> "profile":"sketchy_mad",
> "onlyif":"true",
> "update":{
> "s":"OUTLIER_MAD_ADD(s, value)"
> },
> "init":{
> "s":"OUTLIER_MAD_STATE_MERGE(PROFILE_GET('sketchy_mad','global',
> PROFILE_FIXED(5, 'MINUTES')))"
> },
> "foreach":"'global'",
> "result":"s"
> },
> "op":"add"
> }
> ]
> {code}
> The broken configuration is only discovered after dumping the configuration.
> {code}
> $ bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c PROFILER
> Exception in thread "main" java.lang.RuntimeException: Unable to load {
> "profiles" : {
> "profile" : "sketchy_mad",
> "onlyif" : "true",
> "update" : {
> "s" : "OUTLIER_MAD_ADD(s, value)"
> },
> "init" : {
> "s" : "OUTLIER_MAD_STATE_MERGE(PROFILE_GET('sketchy_mad','global',
> PROFILE_FIXED(5, 'MINUTES')))"
> },
> "foreach" : "'global'",
> "result" : "s"
> }
> }
> at
> org.apache.metron.common.configuration.ConfigurationType.lambda$static$4(ConfigurationType.java:68)
> at
> org.apache.metron.common.configuration.ConfigurationType.deserialize(ConfigurationType.java:93)
> at
> org.apache.metron.common.configuration.ConfigurationsUtils.lambda$dumpConfigs$6(ConfigurationsUtils.java:621)
> at
> org.apache.metron.common.configuration.ConfigurationsUtils.visitConfigs(ConfigurationsUtils.java:575)
> at
> org.apache.metron.common.configuration.ConfigurationsUtils.dumpConfigs(ConfigurationsUtils.java:619)
> at
> org.apache.metron.common.cli.ConfigurationManager.dump(ConfigurationManager.java:189)
> at
> org.apache.metron.common.cli.ConfigurationManager.run(ConfigurationManager.java:268)
> at
> org.apache.metron.common.cli.ConfigurationManager.run(ConfigurationManager.java:243)
> at
> org.apache.metron.common.cli.ConfigurationManager.main(ConfigurationManager.java:355)
> Caused by: org.apache.metron.jackson.databind.JsonMappingException: Can not
> deserialize instance of java.util.ArrayList out of START_OBJECT token
> at [Source: {
> "profiles" : {
> "profile" : "sketchy_mad",
> "onlyif" : "true",
> "update" : {
> "s" : "OUTLIER_MAD_ADD(s, value)"
> },
> "init" : {
> "s" : "OUTLIER_MAD_STATE_MERGE(PROFILE_GET('sketchy_mad','global',
> PROFILE_FIXED(5, 'MINUTES')))"
> },
> "foreach" : "'global'",
> "result" : "s"
> }
> }; line: 2, column: 16] (through reference chain:
> org.apache.metron.common.configuration.profiler.ProfilerConfig["profiles"])
> at
> org.apache.metron.jackson.databind.JsonMappingException.from(JsonMappingException.java:255)
> at
> org.apache.metron.jackson.databind.DeserializationContext.mappingException(DeserializationContext.java:971)
> at
> org.apache.metron.jackson.databind.DeserializationContext.mappingException(DeserializationContext.java:967)
> at
> org.apache.metron.jackson.databind.deser.std.CollectionDeserializer.handleNonArray(CollectionDeserializer.java:327)
> at
> org.apache.metron.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:259)
> at
> org.apache.metron.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:249)
> at
> org.apache.metron.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:26)
> at
> org.apache.metron.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:490)
> at
> org.apache.metron.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:95)
> at
> org.apache.metron.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:260)
> at
> org.apache.metron.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:125)
> at
> org.apache.metron.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3807)
> at
> org.apache.metron.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2797)
> at org.apache.metron.common.utils.JSONUtils.load(JSONUtils.java:79)
> at
> org.apache.metron.common.configuration.ConfigurationType.lambda$static$4(ConfigurationType.java:66)
> ... 8 more
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
