[
https://issues.apache.org/jira/browse/YARN-10003?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17051358#comment-17051358
]
Adam Antal commented on YARN-10003:
-----------------------------------
Hi [~bteke], thanks for working on this!
This might be bugging you at first, but in the Hadoop repository folks prefer
functionality over code style.
This serves multiple purpose:
- One commit is one issue, which resolves one problem - if you fix whitespace
changes that should be technically another issue
- Keeping a clean git history - have a meaningful git blame output for example
- And the most important reason is: cherry-picking to downstream repositories
are always tricky, and introducing non essential whitespace and other types of
needless changes makes it *really* difficult and prone to conflicts. Let's say,
you move {{LogMutation}} class inside {{YarnConfigurationStore}}, and someone
later changes something in that class (an important bugfix). If a downstream
repository wants to incorporate this essential bugfix, but does not want to
have your change (YARN-10003) because it does not need it, cherry-picking can
result in conflicts if git is not aware that you've moved that inner class.
Usually git is very smart to find these things, but the more whitespace changes
you create, the higher the risk you introduce a requirement to a commit
(otherwise cherry-picking some depending commit can result in conflicts which
could have been avoided).
I am aware of the cons as well (worse code quality, accidental mistakes and
typos are hard to fix, it's pretty annoying, etc.) but the general agreement
between Hadoop folks is to discourage these types of changes.
That being said I suggest to remove any whitespace, indentation or non
meaningful change - more concretely:
- Moving the {{LogMutation}} inner class
- Indentation in the parameters of functions
- Extra lines in javadoc
Sometimes IntelliJ auto-format the code that introduced these changes - it can
be turned off in the preferences.
Other than that, the patch seems good. Some thoughts:
- I don't see why we catch the exception in
{{TestInMemoryConfigurationStore#checkVersion}} - if an exception is thrown
test is failed anyways, and the {{Assert#fail}} method does not give further
information.
- Can you add a javadoc for {{YarnConfStoreVersionIncompatibleException}} class?
- Could you please remove the wildcard imports from
{{TestLeveldbConfigurationStore}} and {{TestZKConfigurationStore}} classes?
IntelliJ by default does this auto-import thing. I suggest to turn it off by
setting the corresponding option to 999 in IntelliJ > Preferences > Editor >
Code Style > Java > Imports tab > Class count to use import with '*'.
> YarnConfigurationStore#checkVersion throws exception that belongs to
> RMStateStore
> ---------------------------------------------------------------------------------
>
> Key: YARN-10003
> URL: https://issues.apache.org/jira/browse/YARN-10003
> Project: Hadoop YARN
> Issue Type: Improvement
> Reporter: Szilard Nemeth
> Assignee: Benjamin Teke
> Priority: Major
> Attachments: YARN-10003.001.patch, YARN-10003.002.patch,
> YARN-10003.003.patch
>
>
> RMStateVersionIncompatibleException is thrown from method "checkVersion".
> Moreover, there's a TODO here saying this method is copied from RMStateStore.
> We should revise this method a bit.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
