HDDS-976: Parse network topology from yaml file #661
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
xiaoyuyao
reviewed
Apr 4, 2019
| * @throws IllegalArgumentException xml file content is logically invalid | ||
| */ | ||
| private NodeSchemaLoadResult loadSchemaFromYaml(File schemaFile) { | ||
| LOG.info("Loading network topology layer schema file " + schemaFile); |
Contributor
There was a problem hiding this comment.
NIT: can we use parameterized log4j like below
LOG.info("Loading network topology layer schema file {}", schemaFile);
xiaoyuyao
reviewed
Apr 4, 2019
| NodeSchemaLoadResult finalSchema; | ||
|
|
||
| try { | ||
| FileInputStream fileInputStream = new FileInputStream(schemaFile); |
Contributor
There was a problem hiding this comment.
Can we use try-with-resource to ensure the FileInputStream is closed properly even Exception is thrown on line 210?
xiaoyuyao
reviewed
Apr 4, 2019
| NodeSchemaLoadResult result; | ||
| try { | ||
| result = NodeSchemaLoader.getInstance().loadSchemaFromFile(schemaFile); | ||
| if (schemaFileType.compareTo("yaml") == 0) { |
Contributor
There was a problem hiding this comment.
Can we make the schema type string case insensitive?
xiaoyuyao
reviewed
Apr 4, 2019
| if (schemaFileType.compareTo("yaml") == 0) { | ||
| result = NodeSchemaLoader.getInstance().loadSchemaFromYaml(schemaFile); | ||
| } else { | ||
| result = NodeSchemaLoader.getInstance().loadSchemaFromFile(schemaFile); |
Contributor
There was a problem hiding this comment.
Maybe change this to loadSchemaFromXml?
Contributor
|
Thanks @cjjnjust for working on this. Patch LGTM overall, just few minor issues commented inline. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
Thanks @cjjnjust for the update. Some of the unit test failures seem related, can you check/fix them? |
Contributor
Author
|
@xiaoyuyao , I checked failed tests, it looks like not related to PR, it seems all errors are caused by |
Contributor
|
+1, I will merge/commit the patch shortly. I manually retested the failed tests and they don't repro on my box. |
nandakumar131
pushed a commit
to nandakumar131/hadoop
that referenced
this pull request
Apr 23, 2019
…e Chen. (apache#661) Signed-off-by: Xiaoyu Yao <[email protected]>
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
- Add consistent content for the front-page - Introduce navigation for the overall website - Links not updated yet Author: Jagadish <[email protected]> Reviewers: Jagadish <[email protected]> Closes apache#661 from vjagadish1989/samza-1910
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a early stage patch to support load HDFS schema from a YAML file.
It should need more unit tests.