[
https://issues.apache.org/jira/browse/YARN-9998?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17058734#comment-17058734
]
Adam Antal commented on YARN-9998:
----------------------------------
Hi [~bteke]!
Thanks for the patch. Some general comments:
- Moving class variables falls to the same category just like any other
whitespace change (for the same reasons described
[here|https://issues.apache.org/jira/browse/YARN-10003?focusedCommentId=17051358&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17051358])
So could you move CURRENT_VERSION_INFO back to its original location, and the
restore the following whitespace changes?
{code:java}
@Override
public void initialize(Configuration config, Configuration schedConf,
RMContext rmContext) throws IOException {
{code}
and
{code:java}
@Override
public void confirmMutation(LogMutation pendingMutation,
boolean isValid) {
{code}
- Regarding the parameter indentation during function declaration the
checkstyle does not give a warning about these but now the file is indented
inconsistently (sometimes aligned, sometimes not). It's up to personal
preference, but since we would not like to introduce whitespace changes, could
you align them according to the rest of the file? If you create a new file, you
can align them to your preference (I think we don't hard rules about it).
I was interested about it, and checked the Google Java Style Guide, and it
[discourages the use of horizontal
alignment|https://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment]
due to the fact that its hard to maintain.
- TODOs will be handled in another jira, but let's not commit them along with
this patch. (it may happen that this patch will be cherry-picked or only the
other one independently of each other) IMO every commit should basically stand
on its own if its not part of a bigger feature/epic/task.
And also there is one issue that I had to argue with [~snemeth]. Though in the
original description of the jira it mentions to convert {{compactionTimer}} to
a local variable (just by checking its usages), but I think we should keep it
globally. Actually {{LeveldbConfigurationStore}} has a close function I think
it is reasonable to assume that after the close of the store the timer should
not keep going and doing stuff asynchronously. In short, I think the Timer
should be cancelled in the {{close()}} function of the store. Since this jira
is of type refactor, I urge not to handle it in this jira but to file another
one where we handled this and only this issue (probably covered with a UT).
> Code cleanup in LeveldbConfigurationStore
> -----------------------------------------
>
> Key: YARN-9998
> URL: https://issues.apache.org/jira/browse/YARN-9998
> Project: Hadoop YARN
> Issue Type: Improvement
> Reporter: Szilard Nemeth
> Assignee: Benjamin Teke
> Priority: Minor
> Attachments: YARN-9998.001.patch
>
>
> Many things can be improved:
> * Field compactionTimer could be a local variable
> * Field versiondb should be camelcase
> * initDatabase is a very long method: Initialize db / versionDb should be in
> separate methods, split this method into smaller chunks
> * Remove TODOs
> * Remove duplicated code block in
> LeveldbConfigurationStore.CompactionTimerTask
> * Any other cleanup
--
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
