SAMZA-1143 Include fs.<scheme>.impl.* subkeys to YarnConfiguration used in YarnJobFactory and YarnClusterResourceManager #97
Conversation
| * @param scheme scheme name, such as http, hdfs, myscheme | ||
| * @return a set of subKeys from the configuration without stripping off prefix | ||
| */ | ||
| public Set<String> getFsImplSubKeys(final String scheme) { |
There was a problem hiding this comment.
Prefer an API like
public Map<String, String> getSchemeConfig(final String scheme) {
}
- It makes it more obvious from the API.
- Usually, returning just
keysis of limited value. Almost always, you will want to lookup the value configured for that key. Atleast in all interactions withFsImplConfigin the code base this is true.
|
Thanks @vjagadish1989 ! I have updated based on your comments. |
| * @param scheme scheme name, such as http, hdfs, myscheme | ||
| * @return fs.<scheme>impl | ||
| * @return a set of sub configurations without stripping off prefix |
There was a problem hiding this comment.
nit:
@return config for the particular scheme
set of sub configurations is slightly mis-leading. Probably not worth calling out that the pre-fix is not stripped.
| assertEquals("fs.http.impl", manager.getFsImplKey("http")); | ||
| assertEquals("fs.myscheme.impl", manager.getFsImplKey("myscheme")); | ||
|
|
||
| assertEquals("org.apache.samza.HttpFileSystem", manager.getFsImplClassName("http")); |
There was a problem hiding this comment.
Why do we remove these two assertions?
There was a problem hiding this comment.
Because we removed these two functions, and introduced another function to get the key-value together as a sub-config.
| @@ -61,15 +57,28 @@ public void testNullConfig() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testEmptyImpl() { | |||
There was a problem hiding this comment.
Why are we removing this test for empty implementation?
There was a problem hiding this comment.
It is because we removed getFsImplClassName(), and this test was originally for testing against getFsImplClassName().
|
@fredji97 Looks like there are some conflicts. Can we please resolve them? |
…>.impl, to YarnConfiguration used in YarnJobFactory and YarnClusterResourceManager. When there are additional subconfigurations under fs.<scheme>.impl, such as fs.<scheme>.impl.client, we need to keep the set of configuration completed.
Added support for scala 2.12
Author: Maxim Logvinenko <mlogvinenko@gmail.com>
Reviewers: Jagadish <jagadish@apache.org>,Prateek Maheshwari <prateekm@linkedin.com>
Closes apache#82 from metamx:scala-2.12
…>.impl, to YarnConfiguration used in YarnJobFactory and YarnClusterResourceManager. When there are additional subconfigurations under fs.<scheme>.impl, such as fs.<scheme>.impl.client, we need to keep the set of configuration completed.
This reverts commit fdfeece.
|
@vjagadish1989 thanks I resolved the conflicts. Please let me know if you have any question. |
|
@fredji97 : Thanks for the patch, Merged, and submitted to master. Can you please close the associated JIRA with the fix version? |
SAMZA-1143 Include fs.<scheme>.impl.* subkeys, in addition tofs.<scheme>.impl, to YarnConfiguration used in YarnJobFactory and YarnClusterResourceManager.
When there are additional subconfigurations under fs.myScheme.impl, such as fs.myScheme.impl.client, we need to keep the set of configuration completed in YarnJobFactory and YarnClusterResourceManager. When the context is set for localizing the resource in ClientHelper and YarnContainerRunner, it may rely on this configuration to get the FileStatus information, which may or may not depends on fs.<scheme>.impl and all possible fs.<scheme>.impl.* sub-configuration.
This is an enhanced feature to the PR#90.