Migrating to apache commons configuration 1.10 for parsing ETL jobs c… #851

ShridharSattur · 2017-11-09T23:05:43Z

Migrating to apache commons configuration 1.10 for Parsing ETL jobs congifuration , It will increase the readability aspect of job configs and also wherehows can leverage apache common configuration features. Below is one of the usage

Instead of providing comma separated values for job properties one can define the same properties in multiple lines
ex: mysql_db = db1,db2,db3
with this change we can define it as
mysql_db = db1
mysql_db=db2
mysql_db=db3

coveralls · 2017-11-09T23:20:16Z

Coverage remained the same at 27.444% when pulling 82d2995 on ShridharSattur:commons into ae801c8 on linkedin:master.

mars-lan · 2017-11-10T00:39:18Z

gradle/scripts/dependency.gradle

@@ -19,6 +19,8 @@ ext.externalDependency = [
    "jsoup"               : "org.jsoup:jsoup:1.8.3",
    "commons_io"          : "commons-io:commons-io:2.5",
    "commons_lang3"       : "org.apache.commons:commons-lang3:3.6",
+    "commons_codec"       : "commons-codec:commons-codec:1.10",


Is this really needed?

Removed now, used it earlier just in case if i had to encode the reader object.

mars-lan · 2017-11-10T00:39:55Z

wherehows-common/src/main/java/wherehows/common/utils/JobsUtil.java

-      .add(Pattern.compile("\\$(.+)")) // $ENV_VAR
-      .add(Pattern.compile("\\$\\{(.+)\\}")) // ${ENV_VAR}
-      .build();
+  private static final List<Pattern> ENV_VAR_PATTERNS =


Should be able to drop this now?

Yes removed it along with method and test

mars-lan · 2017-11-10T00:41:48Z

wherehows-common/src/test/java/wherehows/common/utils/JobsUtilTest.java

+    Properties props = new Properties();
+    Path path = createPropertiesFile(propertyStr);
+
+    for (File file : new File(dir).listFiles()) {


Why? Don't you already have the path from the previous line?

Updated it now.

coveralls · 2017-11-10T01:14:37Z

Coverage remained the same at 27.444% when pulling c758088 on ShridharSattur:commons into 62b60de on linkedin:master.

coveralls · 2017-11-10T01:19:55Z

Coverage remained the same at 27.444% when pulling c758088 on ShridharSattur:commons into 62b60de on linkedin:master.

mars-lan · 2017-11-10T01:18:17Z

wherehows-common/src/test/java/wherehows/common/utils/JobsUtilTest.java

+  public void testMultipleLinesSameProperties() throws IOException, ConfigurationException {
+    String dir = "/tmp/";
+    String propertyStr =
+        "var1=foo\n" + "var1=foo\n" + "var1=foo\n" + "var2=foo\n" + "var2=foo\n" + "var3=foo\n" + "var1=foo\n"


Let's add a few variations to the values

I have added few tests and enhanced this test.

What I meant is not to use "foo" for all values

mars-lan · 2017-11-10T01:19:51Z

wherehows-common/src/test/java/wherehows/common/utils/JobsUtilTest.java

+
+    Assert.assertNotEquals(props, null);
+    Assert.assertEquals(props.getProperty("var1", ""), "foo,foo,foo,foo");
+    Assert.assertTrue(props.getProperty("var2").length() > 0);


Let's do simple literal value comparison for var2, var3, & var4.

Did literal value comparison now.

mars-lan · 2017-11-10T01:21:22Z

wherehows-common/src/test/java/wherehows/common/utils/JobsUtilTest.java

@@ -30,23 +31,7 @@
 public class JobsUtilTest {

  @Test
-  public void testEnvVarResolution() throws IOException {


May still want to verify that env vars are resolved properly?

Good point , i have added back earlier code which used to resolve environment variable. In my next PR, i'm going to modify resolving environmental variables using apache commons configuration.

coveralls · 2017-11-10T03:56:25Z

Coverage remained the same at 27.444% when pulling b8e4cab on ShridharSattur:commons into 668e631 on linkedin:master.

coveralls · 2017-11-10T21:52:50Z

Coverage remained the same at 27.444% when pulling e00edef on ShridharSattur:commons into 668e631 on linkedin:master.

coveralls · 2017-11-10T21:55:15Z

Coverage remained the same at 27.444% when pulling e00edef on ShridharSattur:commons into 668e631 on linkedin:master.

mars-lan · 2017-11-13T18:28:11Z

wherehows-common/src/test/java/wherehows/common/utils/JobsUtilTest.java

+    String propertyStr =
+        "var1=foo\n" + "var1=foo\n" + "var1=foo\n" + "var2=foo\n" + "var2=foo\n" + "var3=foo\n" + "var1=foo\n"
+            + "var4=foo\n";
+    String propertyStr1 = "var5=foo,bar,test,test1\n" + "var5=test2\n";


Just append this to propertyStr directly instead of creating another variable?

mars-lan · 2017-11-13T18:28:39Z

wherehows-common/src/test/java/wherehows/common/utils/JobsUtilTest.java

+    Assert.assertNotEquals(props, null);
+    Assert.assertEquals(props.getProperty("var1", ""), "foo,foo,foo,foo");
+    Assert.assertEquals(props.getProperty("var2"), "foo,foo");
+    Assert.assertEquals(props.getProperty("var3"), props.getProperty("var4"));


ssert.assertEquals(props.getProperty("var3"), "foo")
ssert.assertEquals(props.getProperty("var4"), "foo")

Got it , verified all the literal values now.

mars-lan · 2017-11-13T18:28:55Z

wherehows-common/src/test/java/wherehows/common/utils/JobsUtilTest.java

+  public void testMultipleLinesSameProperties() throws IOException, ConfigurationException {
+    String dir = "/tmp/";
+    String propertyStr =
+        "var1=foo\n" + "var1=foo\n" + "var1=foo\n" + "var2=foo\n" + "var2=foo\n" + "var3=foo\n" + "var1=foo\n"


What I meant is not to use "foo" for all values

…onfigurations.

coveralls · 2017-11-13T19:04:32Z

Coverage remained the same at 27.444% when pulling 4b9ea10 on ShridharSattur:commons into 0aa6f79 on linkedin:master.

mars-lan reviewed Nov 10, 2017

View reviewed changes

ShridharSattur force-pushed the commons branch 2 times, most recently from 81df270 to c758088 Compare November 10, 2017 01:07

mars-lan reviewed Nov 10, 2017

View reviewed changes

ShridharSattur force-pushed the commons branch from c758088 to b8e4cab Compare November 10, 2017 03:43

ShridharSattur force-pushed the commons branch 2 times, most recently from 5d84d37 to e00edef Compare November 10, 2017 21:41

mars-lan reviewed Nov 13, 2017

View reviewed changes

Migrating to apache commons configuration 1.10 for parsing ETL jobs c…

4b9ea10

…onfigurations.

ShridharSattur force-pushed the commons branch from e00edef to 4b9ea10 Compare November 13, 2017 18:51

mars-lan approved these changes Nov 14, 2017

View reviewed changes

mars-lan merged commit f1b61d6 into datahub-project:master Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating to apache commons configuration 1.10 for parsing ETL jobs c… #851

Migrating to apache commons configuration 1.10 for parsing ETL jobs c… #851

ShridharSattur commented Nov 9, 2017

coveralls commented Nov 9, 2017

mars-lan Nov 10, 2017

ShridharSattur Nov 10, 2017

mars-lan Nov 10, 2017

ShridharSattur Nov 10, 2017

mars-lan Nov 10, 2017

ShridharSattur Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

mars-lan Nov 10, 2017

ShridharSattur Nov 10, 2017

mars-lan Nov 13, 2017

mars-lan Nov 10, 2017

ShridharSattur Nov 10, 2017

mars-lan Nov 10, 2017

ShridharSattur Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

mars-lan Nov 13, 2017

ShridharSattur Nov 13, 2017

mars-lan Nov 13, 2017

ShridharSattur Nov 13, 2017

mars-lan Nov 13, 2017

coveralls commented Nov 13, 2017

Migrating to apache commons configuration 1.10 for parsing ETL jobs c… #851

Migrating to apache commons configuration 1.10 for parsing ETL jobs c… #851

Conversation

ShridharSattur commented Nov 9, 2017

coveralls commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 13, 2017