You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Peter Cseh <ge...@cloudera.com> on 2017/03/16 09:32:30 UTC
Review Request 57680: OOZIE-2812 SparkConfigurationService should
support loading configurations from multiple Spark versions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57680/
-----------------------------------------------------------
Review request for oozie.
Bugs: OOZIE-2812
https://issues.apache.org/jira/browse/OOZIE-2812
Repository: oozie-git
Description
-------
OOZIE-2812
SparkConfigurationService should support loading configurations from multiple Spark versions
Diffs
-----
core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java 1a3197abb52f80b545dcc8634a8cac7a281a9eac
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java b15cce0d6813375778abf1d2bbe09e83734d19f0
core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java 0e00a457890d1f4b8e86c19043cd597928cdd9bb
Diff: https://reviews.apache.org/r/57680/diff/1/
Testing
-------
Thanks,
Peter Cseh
Re: Review Request 57680: OOZIE-2812 SparkConfigurationService should
support loading configurations from multiple Spark versions
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57680/#review169109
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java
Lines 82 (patched)
<https://reviews.apache.org/r/57680/#comment241428>
`sharelibName` would be a better name
core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java
Lines 82-88 (original), 83-89 (patched)
<https://reviews.apache.org/r/57680/#comment241427>
tabs vs spaces
core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java
Line 83 (original), 84 (patched)
<https://reviews.apache.org/r/57680/#comment241429>
What about extracting `SparkConfigurationService` to a local variable? See Law of Demeter
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 39-47 (original), 39-47 (patched)
<https://reviews.apache.org/r/57680/#comment241434>
Honestly, either more descriptive constant names, or javadoc, or a neatly named builder would be of great value here.
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Line 40 (original), 40 (patched)
<https://reviews.apache.org/r/57680/#comment241432>
Why not call that `CONFIGURATIONS_SUFFIX` instead?
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Line 43 (original), 43 (patched)
<https://reviews.apache.org/r/57680/#comment241433>
Why not call that rather `BLACKLIST_SUFFIX`?
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Line 48 (original), 48 (patched)
<https://reviews.apache.org/r/57680/#comment241430>
Every time I see something like `Map<String, Map<String, Something>>` reminds me always to use Guava's `Multimap<String, Something>`: https://github.com/google/guava/wiki/NewCollectionTypesExplained#multimap
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 54 (patched)
<https://reviews.apache.org/r/57680/#comment241439>
Are you sure `^` is not needed as a prefix, and `$` is needed at the end? It would be best to have some unit tests that show what regexps are accepted and which ones not.
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 55 (patched)
<https://reviews.apache.org/r/57680/#comment241443>
`sparkConfigName`
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 76 (patched)
<https://reviews.apache.org/r/57680/#comment241440>
What about using `LinkedHashSet` which preserves addition order?
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 77 (patched)
<https://reviews.apache.org/r/57680/#comment241444>
`blacklistedPropertyName`
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 81-102 (original), 89-114 (patched)
<https://reviews.apache.org/r/57680/#comment241445>
This functionality should really be extracted to a separate `SparkConfigLoader` class - `SparkConfigurationService` does way too much ATM.
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 119-123 (original), 131-135 (patched)
<https://reviews.apache.org/r/57680/#comment241453>
Should also be extracted to a separate class.
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java
Lines 145-153 (original), 160-168 (patched)
<https://reviews.apache.org/r/57680/#comment241454>
tabs vs spaces
core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java
Line 50 (original), 50 (patched)
<https://reviews.apache.org/r/57680/#comment241456>
This test case is really undreadable. Can you please split that up to multiple test cases and add new ones that cover your new regex and different kinds of Spark configuration mixes that cover the use cases we'd like to address: Spark 1.6 standalone, Spark 2.1 standalone, Spark 1.6 w/ Spark 2.0 w/ Spark 2.1, etc.
core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java
Line 63 (original), 63 (patched)
<https://reviews.apache.org/r/57680/#comment241455>
Maybe extract `"spark"` to a constant?
core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java
Lines 160 (patched)
<https://reviews.apache.org/r/57680/#comment241458>
From the test method name alone I cannot conclude what is the test case performing and what are the expected outcomes / end states. Please rename it.
core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java
Lines 161-163 (patched)
<https://reviews.apache.org/r/57680/#comment241459>
Wow, I cannot really understand who is who, the sheer amount of one-letter constants is amazing.
core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java
Lines 173-175 (patched)
<https://reviews.apache.org/r/57680/#comment241461>
Maybe use the same Builder here, or `String.format()` w/ the same constant would be less copy-and-paste-error prone.
- Andr�s Piros
On March 16, 2017, 9:32 a.m., Peter Cseh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57680/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 9:32 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2812
> https://issues.apache.org/jira/browse/OOZIE-2812
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2812
> SparkConfigurationService should support loading configurations from multiple Spark versions
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java 1a3197abb52f80b545dcc8634a8cac7a281a9eac
> core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java b15cce0d6813375778abf1d2bbe09e83734d19f0
> core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java 0e00a457890d1f4b8e86c19043cd597928cdd9bb
>
>
> Diff: https://reviews.apache.org/r/57680/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Cseh
>
>