[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user asfgit closed the pull request at: https://github.com/apache/metron/pull/851 ---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r155347462
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -572,15 +590,22 @@ public static void
applyConfigPatchToZookeeper(ConfigurationType configurationTy
* @param patchData a JSON patch in the format specified by RFC 6902
* @param client access to zookeeeper
*/
- public static void applyConfigPatchToZookeeper(ConfigurationType
configurationType,
- Optional configName,
- byte[] patchData, CuratorFramework client) throws Exception {
+ public static void applyConfigPatchToZookeeper(
+ ConfigurationType configurationType,
+ Optional configName,
+ byte[] patchData, CuratorFramework client) throws Exception {
+
byte[] configData = readConfigBytesFromZookeeper(configurationType,
configName, client);
JsonNode source = JSONUtils.INSTANCE.readTree(configData);
JsonNode patch = JSONUtils.INSTANCE.readTree(patchData);
JsonNode patchedConfig = JSONUtils.INSTANCE.applyPatch(patch, source);
-writeConfigToZookeeper(configurationType, configName,
-JSONUtils.INSTANCE.toJSONPretty(patchedConfig), client);
+byte[] prettyPatchedConfig =
JSONUtils.INSTANCE.toJSONPretty(patchedConfig);
+
+// ensure the patch produces a valid result; otherwise exception
thrown during deserialization
--- End diff --
Heh, well our meta-PR was to address a code change that resulted in
duplication, not just a style preference. ;)
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154824244
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -343,25 +343,42 @@ 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
--- End diff --
METRON-1342
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154824031
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -343,25 +343,42 @@ 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
--- End diff --
I'll open a jira
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154821946
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -343,25 +343,42 @@ 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
--- End diff --
Yeah I get it. ;). It wasn't my preference to include all of that here, but
I bend to make people happy.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154804149
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -572,15 +590,22 @@ public static void
applyConfigPatchToZookeeper(ConfigurationType configurationTy
* @param patchData a JSON patch in the format specified by RFC 6902
* @param client access to zookeeeper
*/
- public static void applyConfigPatchToZookeeper(ConfigurationType
configurationType,
- Optional configName,
- byte[] patchData, CuratorFramework client) throws Exception {
+ public static void applyConfigPatchToZookeeper(
+ ConfigurationType configurationType,
+ Optional configName,
+ byte[] patchData, CuratorFramework client) throws Exception {
+
byte[] configData = readConfigBytesFromZookeeper(configurationType,
configName, client);
JsonNode source = JSONUtils.INSTANCE.readTree(configData);
JsonNode patch = JSONUtils.INSTANCE.readTree(patchData);
JsonNode patchedConfig = JSONUtils.INSTANCE.applyPatch(patch, source);
-writeConfigToZookeeper(configurationType, configName,
-JSONUtils.INSTANCE.toJSONPretty(patchedConfig), client);
+byte[] prettyPatchedConfig =
JSONUtils.INSTANCE.toJSONPretty(patchedConfig);
+
+// ensure the patch produces a valid result; otherwise exception
thrown during deserialization
--- End diff --
Never mind Nick, I didn't read this correctly, and I have the
"serialization mixed with logical validation" stuff upfront in my head because
of my work over in https://github.com/apache/metron/pull/856.
Sorry
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154803527
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -572,15 +590,22 @@ public static void
applyConfigPatchToZookeeper(ConfigurationType configurationTy
* @param patchData a JSON patch in the format specified by RFC 6902
* @param client access to zookeeeper
*/
- public static void applyConfigPatchToZookeeper(ConfigurationType
configurationType,
- Optional configName,
- byte[] patchData, CuratorFramework client) throws Exception {
+ public static void applyConfigPatchToZookeeper(
+ ConfigurationType configurationType,
+ Optional configName,
+ byte[] patchData, CuratorFramework client) throws Exception {
+
byte[] configData = readConfigBytesFromZookeeper(configurationType,
configName, client);
JsonNode source = JSONUtils.INSTANCE.readTree(configData);
JsonNode patch = JSONUtils.INSTANCE.readTree(patchData);
JsonNode patchedConfig = JSONUtils.INSTANCE.applyPatch(patch, source);
-writeConfigToZookeeper(configurationType, configName,
-JSONUtils.INSTANCE.toJSONPretty(patchedConfig), client);
+byte[] prettyPatchedConfig =
JSONUtils.INSTANCE.toJSONPretty(patchedConfig);
+
+// ensure the patch produces a valid result; otherwise exception
thrown during deserialization
--- End diff --
```java
//Some configurations have side effects to deserialization, and may throw
exceptions from
//validation that may happen during the deserialization. This does not
meant that the //deserialization has failed due to invalid serialization.
```
?
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154799152
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -572,15 +590,22 @@ public static void
applyConfigPatchToZookeeper(ConfigurationType configurationTy
* @param patchData a JSON patch in the format specified by RFC 6902
* @param client access to zookeeeper
*/
- public static void applyConfigPatchToZookeeper(ConfigurationType
configurationType,
- Optional configName,
- byte[] patchData, CuratorFramework client) throws Exception {
+ public static void applyConfigPatchToZookeeper(
+ ConfigurationType configurationType,
+ Optional configName,
+ byte[] patchData, CuratorFramework client) throws Exception {
+
byte[] configData = readConfigBytesFromZookeeper(configurationType,
configName, client);
JsonNode source = JSONUtils.INSTANCE.readTree(configData);
JsonNode patch = JSONUtils.INSTANCE.readTree(patchData);
JsonNode patchedConfig = JSONUtils.INSTANCE.applyPatch(patch, source);
-writeConfigToZookeeper(configurationType, configName,
-JSONUtils.INSTANCE.toJSONPretty(patchedConfig), client);
+byte[] prettyPatchedConfig =
JSONUtils.INSTANCE.toJSONPretty(patchedConfig);
+
+// ensure the patch produces a valid result; otherwise exception
thrown during deserialization
--- End diff --
I'm not sure exactly what you're looking for. Can you suggest some
specific text?
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154798328
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -343,25 +343,42 @@ 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
--- End diff --
Sure, but this method signature already existed. Do I need to fix that on
this PR?
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154761355
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -343,25 +343,42 @@ 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
--- End diff --
When reading from file, the API should explicitly require file Paths as
opposed to just strings.
It makes the api methods easier to understand when there are a bunch of
similar methods.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154763112
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -572,15 +590,22 @@ public static void
applyConfigPatchToZookeeper(ConfigurationType configurationTy
* @param patchData a JSON patch in the format specified by RFC 6902
* @param client access to zookeeeper
*/
- public static void applyConfigPatchToZookeeper(ConfigurationType
configurationType,
- Optional configName,
- byte[] patchData, CuratorFramework client) throws Exception {
+ public static void applyConfigPatchToZookeeper(
+ ConfigurationType configurationType,
+ Optional configName,
+ byte[] patchData, CuratorFramework client) throws Exception {
+
byte[] configData = readConfigBytesFromZookeeper(configurationType,
configName, client);
JsonNode source = JSONUtils.INSTANCE.readTree(configData);
JsonNode patch = JSONUtils.INSTANCE.readTree(patchData);
JsonNode patchedConfig = JSONUtils.INSTANCE.applyPatch(patch, source);
-writeConfigToZookeeper(configurationType, configName,
-JSONUtils.INSTANCE.toJSONPretty(patchedConfig), client);
+byte[] prettyPatchedConfig =
JSONUtils.INSTANCE.toJSONPretty(patchedConfig);
+
+// ensure the patch produces a valid result; otherwise exception
thrown during deserialization
--- End diff --
Can we reword this? It needs to be clear that some things cannot be
deserialized if they have an invalid configuration.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154663544
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
The meta-PR is merged
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154508609
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
Haha, I know. I couldn't resist seeing if GitHub actually allowed this sort
of thing, and well, it was actually pretty straightforward. I don't care if we
pull it all together or submit them separately. It sounds like we're all in
agreement that the refactoring is a good idea, so if it was split them Casey's
interface refactoring would go in immediately following with the necessary
votes. I'll leave it to you both to decide.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154508202
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
I like the refactoring, guys. It's looking food. Can't believe we have a
PR of a PR of this PR. One more level and I think we hit Inception's limbo.
I'd be open to including those changes here or even just creating a
separate PR for the refactoring work as a follow-on to this. That refactoring
adds a good bit of code in itself. Maybe we could even tackle some further
ConfigurationUtils clean-up in that same PR.
But If we want @cestella and @mmiklavc 's refactoring to go in with this
PR, then we need @cestella to do the first "kick" and merge @mmiklavc's PR.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154461563
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
@mmiklavc I'm ok with that modification if we decide to go this route.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154461007
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
@cestella I submitted a PR against your PR on this PR -
https://github.com/cestella/incubator-metron/pull/8
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154231709
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
One more thought - the code for option 3 does add more code than the
copy/paste, but in spite of that I think it puts us closer to being able to
delete a lot of the redundant and/or dead code in config utils. So I think it's
a net positive.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154215790
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
Sounds good. Just so I'm clear about my preferred approach (option 3
there), I went ahead and implemented it in a
[PR](https://github.com/nickwallen/metron/pull/12) against your PR so you can
see what I was talking about.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154209165
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
Your first suggestion with the callback just seems less obvious and clear
to me (IMHO).
I'll try and think through your 2nd and 3rd suggestions, but (at least
right now) I'm not seeing something that doesn't add more complexity to
ConfigurationUtils.
But clearly I could be totally wrong here. Let me think on it.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154208493
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
Yes, I had the same thought and tried for a bit to refactor it. I landed
on this because the various other ways to do this either (1) seemed more
complex and less obvious as to what we are actually doing here or (2) lead down
a path of heavy refactoring of ConfigurationUtils. Both of which i wanted to
avoid
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154208318
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -156,7 +156,7 @@ public static void
writeConfigToZookeeper(ConfigurationType configType,Optional<
}
private static String getConfigZKPath(ConfigurationType configType,
Optional configName) {
-String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName : "";
+String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName.get() : "";
--- End diff --
Yeah, this is not one of those things that can be done as a side-effect of
another PR. I considered refactoring it as part of the zookeeper refactoring
that I did earlier. It needs more direct attention and refactoring.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154207865
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -156,7 +156,7 @@ public static void
writeConfigToZookeeper(ConfigurationType configType,Optional<
}
private static String getConfigZKPath(ConfigurationType configType,
Optional configName) {
-String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName : "";
+String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName.get() : "";
--- End diff --
I think that's because we handle global config in a special way. This whole
class needs refactored, and I'd have done more with the original patch PR had
it not blown up scope.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154207614
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -156,7 +156,7 @@ public static void
writeConfigToZookeeper(ConfigurationType configType,Optional<
}
private static String getConfigZKPath(ConfigurationType configType,
Optional configName) {
-String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName : "";
+String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName.get() : "";
--- End diff --
We need to refactor `ConfigurationUtils` so badly that it hurts.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154207266
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java
---
@@ -156,7 +156,7 @@ public static void
writeConfigToZookeeper(ConfigurationType configType,Optional<
}
private static String getConfigZKPath(ConfigurationType configType,
Optional configName) {
-String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName : "";
+String pathSuffix = configName.isPresent() && configType != GLOBAL ?
"/" + configName.get() : "";
--- End diff --
Yeah, this confused me too. There are a lot of paths through
ConfigurationUtils. Some paths worked, others didn't.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154206090
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
Oh, an even better option (IMO) would be to add a
`writeSensorConfigToZookeeper(String sensorType, byte[] configData,
CuratorFramework client)` method to `ConfigurationType`. Anyway, I'll stop ;)
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154204458
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
The other option here is to add a new method called
`writeSensorConfigToZookeeper(ConfigurationType type, String sensorType, byte[]
configData, CuratorFramework client)` that calls the appropriate
`writeSensorXConfigToZookeeper` call.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/851#discussion_r154202685
--- 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 configName) throws
Exception {
+ public static void uploadConfigsToZookeeper(
+ String rootFilePath,
+ CuratorFramework client,
+ ConfigurationType type,
+ Optional 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 sensorIndexingConfigs =
readSensorConfigsFromFile(rootFilePath, type,
-configName);
-for (String sensorType : sensorIndexingConfigs.keySet()) {
- writeConfigToZookeeper(type, configName,
sensorIndexingConfigs.get(sensorType), client);
+
+ case PARSER: {
+Map configs =
readSensorConfigsFromFile(rootFilePath, PARSER, configName);
--- End diff --
These cases look like they are cut and pasted which seems like code smell
to me and might be a maintenance issue. Can we extract the common code for
Parser, Enrichment, and Indexing into a separate function that is called here?
Perhaps something like:
```
void writeSensorConfigs(ConfigurationType type, Optional
configName, BiFunction callback) {
Map configs = readSensorConfigsFromFile(rootFilePath,
type, configName);
for(String sensorType : configs.keySet()) {
byte[] configData = configs.get(sensorType);
callback.apply(sensorType, configData);
}
}
```
which could be called from this case via `writeSensorConfigs(PARSER,
configName, (sensorType, configData) ->
writeSensorParserConfigToZookeeper(sensorType, configData, client));`
Take the above as a very rough suggestion and you can feel to abstract it
however you wish.
---
[GitHub] metron pull request #851: METRON-1336 Patching Can Result in Bad Configurati...
GitHub user nickwallen opened a pull request:
https://github.com/apache/metron/pull/851
METRON-1336 Patching Can Result in Bad Configuration
The following problems are addressed in this PR.
* A patch can be constructed that when applied creates an invalid
configuration. The invalid configuration is not discovered until attempting to
"pull" it back out of Zookeeper. In all cases, the result of applying a patch
needs to be validated before pushing it to Zookeeper.
* The Profiler configuration can only be pushed when pushing all
configurations at once. Attempting to push just the Profiler configuration
alone, fails silently.
* Added unit tests to validate that a 'bad' patch cannot be applied.
* Added a good number of unit tests to ensure invalid configurations cannot
be pushed under all circumstances (parser, enrichment, indexing, profiler,
"push all").
* Added a test to ensure that the Profiler configuration can be pushed
independently.
## Manual Testing
1. Launch Full Dev.
1. Environment definitions that will be used later.
```
export METRON_HOME=/usr/metron/0.4.2
export ZOOKEEPER=node1:2181
```
1. Dump all existing configurations.
```
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP
```
1. Dump just the Profiler configuration. There should be none.
```
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c PROFILER
```
1. Create a Profiler configuration on disk.
```
$ cat $METRON_HOME/config/profiler.json
{
"profiles": [
{
"profile": "hello-world",
"onlyif": "exists(ip_src_addr)",
"foreach": "ip_src_addr",
"init":{ "count": "0" },
"update": { "count": "count + 1" },
"result": "count"
}
]
}
```
1. Push the Profiler configuration.
```
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PUSH -c PROFILER
-i $METRON_HOME/config
```
1. Dump just the Profiler configuration. It should be defined now.
```
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c PROFILER
```
1. Create a patch for the Profiler configuration.
```
$ cat profile.patch
[
{
"op": "add",
"path": "/profiles/0/profile",
"value": "changed-profile-name"
}
]
```
1. Apply the patch.
```
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c PROFILER
-pf profile.patch
```
1. Dump the Profiler configuration and ensure that the name of the Profile
was changed.
```
[root@node1 ~]# $METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m
DUMP -c PROFILER
PROFILER Config: profiler
{
"profiles" : [ {
"profile" : "changed-profile-name",
"onlyif" : "exists(ip_src_addr)",
"foreach" : "ip_src_addr",
"init" : {
"count" : "0"
},
"update" : {
"count" : "count + 1"
},
"result" : "count"
} ]
}
```
1. Create a patch that would make the Profiler configuration invalid.
```
[root@node1 ~]# cat bad.patch
[
{
"op": "add",
"path": "/profiles/0/invalid",
"value": "22"
}
]
```
1. Apply the bad patch. The script should terminate with an error.
```
[root@node1 ~]# $METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m
PATCH -c PROFILER -pf bad.patch
2017-11-28 17:37:17 ERROR ConfigurationManager:314 - Unable to apply
patch to Zookeeper config
java.lang.RuntimeException: Unable to load {
"profiles" : [ {
"profile" : "changed-profile-name",
"onlyif" : "exists(ip_src_addr)",
"foreach" : "ip_src_addr",
"init" : {
"count" : "0"
},
"update" : {
"count" : "count + 1"
},
"result" : "count",
"invalid" : "22"
} ]
}
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.applyConfigPatchToZookeeper(ConfigurationsUtils.java:621)
at
