[
https://issues.apache.org/jira/browse/YARN-9519?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16833662#comment-16833662
]
Szilard Nemeth commented on YARN-9519:
--------------------------------------
Hi [~adam.antal]!,
Thanks for this patch!
1. In testDefaultLogAggregationFileControllerFactory:
{code:java}
LinkedList<LogAggregationFileController> list = factory
.getConfiguredLogAggregationFileControllerList();
{code}
Please use a more descriptive name rather than "list" for the variable.
You should also use the type List instead of LinkedList (the broader the type
is the better).
I'm also seeing some similar "unnecessarily narrow" types for LinkedList across
the class.
2. You should use instanceof checks more consistently:
You have 2 types of checks across the test class. Can we consolidate this into
one type of assertion?
{code:java}
LinkedList<LogAggregationFileController> list = factory
.getConfiguredLogAggregationFileControllerList();
{code}
{code:java}
assertTrue(factory.getFileControllerForWrite()
instanceof LogAggregationTFileController);
{code}
3. In general: Can you please add messsages for the asserts?
4. In testLogAggregationFileControllerFactoryClassNotSet: I'm always preferring
to use the ExpectedException JUnit Rule for more clear exception assertions,
rather than catching the exception.
See [this article|https://www.baeldung.com/junit-assert-exception] for details
5. I'm confused with this test method name: testClassConfUsed
Please use configuration instead for clarity for every occasion :)
Otherwise, the tests look good to me.
> TFile log aggregation file format is insensitive to the
> yarn.log-aggregation.TFile.remote-app-log-dir config
> ------------------------------------------------------------------------------------------------------------
>
> Key: YARN-9519
> URL: https://issues.apache.org/jira/browse/YARN-9519
> Project: Hadoop YARN
> Issue Type: Bug
> Components: log-aggregation
> Affects Versions: 3.2.0
> Reporter: Adam Antal
> Assignee: Adam Antal
> Priority: Major
> Attachments: YARN-9519.001.patch, YARN-9519.002.patch
>
>
> The TFile log aggregation file format is not sensitive to the
> yarn.log-aggregation.TFile.remote-app-log-dir config.
> In {{LogAggregationTFileController$initInternal}}:
> {code:java}
> this.remoteRootLogDir = new Path(
> conf.get(YarnConfiguration.NM_REMOTE_APP_LOG_DIR,
> YarnConfiguration.DEFAULT_NM_REMOTE_APP_LOG_DIR));
> {code}
> So the remoteRootLogDir is only aware of the
> yarn.nodemanager.remote-app-log-dir config, while other file format, like
> IFile defaults to the file format config, so its priority is higher.
> From {{LogAggregationIndexedFileController$initInternal}}:
> {code:java}
> String remoteDirStr = String.format(
> YarnConfiguration.LOG_AGGREGATION_REMOTE_APP_LOG_DIR_FMT,
> this.fileControllerName);
> String remoteDir = conf.get(remoteDirStr);
> if (remoteDir == null || remoteDir.isEmpty()) {
> remoteDir = conf.get(YarnConfiguration.NM_REMOTE_APP_LOG_DIR,
> YarnConfiguration.DEFAULT_NM_REMOTE_APP_LOG_DIR);
> }
> {code}
> (Where these configs are: )
> {code:java}
> public static final String LOG_AGGREGATION_REMOTE_APP_LOG_DIR_FMT
> = YARN_PREFIX + "log-aggregation.%s.remote-app-log-dir";
> public static final String NM_REMOTE_APP_LOG_DIR =
> NM_PREFIX + "remote-app-log-dir";
> {code}
> I suggest TFile should try to obtain the remote dir config from
> yarn.log-aggregation.TFile.remote-app-log-dir first, and only if that is not
> specified falls back to the yarn.nodemanager.remote-app-log-dir config.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
