[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15369734#comment-15369734 ] Hudson commented on YARN-3390: -- SUCCESS: Integrated in Hadoop-trunk-Commit #10074 (See [https://builds.apache.org/job/Hadoop-trunk-Commit/10074/]) YARN-3390. Reuse TimelineCollectorManager for RM (Zhijie Shen via sjlee) (sjlee: rev 11e8905d8daf129afb6fe2e5a0eca11bcb1719c8) * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/collector/TestTimelineCollectorManager.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMContextImpl.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/timelineservice/RMTimelineCollectorManager.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/main/java/org/apache/hadoop/yarn/server/timelineservice/collector/AppLevelTimelineCollector.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/main/java/org/apache/hadoop/yarn/server/timelineservice/collector/NodeTimelineCollectorManager.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/main/java/org/apache/hadoop/yarn/server/timelineservice/collector/TimelineCollector.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/timelineservice/RMTimelineCollector.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/TestTimelineServiceClientIntegration.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/RMAppImpl.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/collector/TestPerNodeTimelineCollectorsAuxService.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/main/java/org/apache/hadoop/yarn/server/timelineservice/collector/PerNodeTimelineCollectorsAuxService.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/main/java/org/apache/hadoop/yarn/server/timelineservice/collector/TimelineCollectorWebService.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/main/java/org/apache/hadoop/yarn/server/timelineservice/collector/TimelineCollectorManager.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMActiveServiceContext.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMContext.java * hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/collector/TestNMTimelineCollectorManager.java > Reuse TimelineCollectorManager for RM > - > > Key: YARN-3390 > URL: https://issues.apache.org/jira/browse/YARN-3390 > Project: Hadoop YARN > Issue Type: Sub-task > Components: timelineserver >Reporter: Zhijie Shen >Assignee: Zhijie Shen > Fix For: YARN-2928 > > Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch, > YARN-3390.4.patch > > > RMTimelineCollector should have the context info of each app whose entity > has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14511962#comment-14511962 ] Zhijie Shen commented on YARN-3390: --- [~sjlee0], [~gtCarrera9] and [~djp], given the discussion about the race of collector map is not settled, can we separate it in another jira, move on with v3 patch, and unblock this jira for RM writing timeline data? Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14512056#comment-14512056 ] Sangjin Lee commented on YARN-3390: --- The latest patch LGTM. I'll commit it shortly. Thanks for your patience! Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch, YARN-3390.4.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[
https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14512013#comment-14512013
]
Sangjin Lee commented on YARN-3390:
---
It's still v.3 (as it includes a couple of other changes over v.2) but
putIfAbsent() should be basically back to v.2 (LOG.error should still be
LOG.info). In other words,
{code}
public TimelineCollector putIfAbsent(ApplicationId appId,
TimelineCollector collector) {
TimelineCollector collectorInTable = null;
synchronized (collectors) {
collectorInTable = collectors.get(appId);
if (collectorInTable == null) {
try {
// initialize, start, and add it to the collection so it can be
// cleaned up when the parent shuts down
collector.init(getConfig());
collector.start();
collectors.put(appId, collector);
LOG.info(the collector for + appId + was added);
collectorInTable = collector;
postPut(appId, collectorInTable);
} catch (Exception e) {
throw new YarnRuntimeException(e);
}
} else {
LOG.info(the collector for + appId + already exists!);
}
}
return collectorInTable;
}
{code}
How's that?
Reuse TimelineCollectorManager for RM
-
Key: YARN-3390
URL: https://issues.apache.org/jira/browse/YARN-3390
Project: Hadoop YARN
Issue Type: Sub-task
Components: timelineserver
Reporter: Zhijie Shen
Assignee: Zhijie Shen
Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch
RMTimelineCollector should have the context info of each app whose entity
has been put
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14511951#comment-14511951 ] Li Lu commented on YARN-3390: - Hi [~sjlee0], I think the racy situation happens like this: thread 1 and 2 try to update the collector for the same appId. Thread 1 arrived first, performs the putIfAbsent with the incoming collector and succeed. Thread 2 then come and grab the collector put by thread 1. If thread 1 is blocked for some reason, thread 2 will operate on the uninitialized collector, which may cause potential problems. I'm not sure how severe this problem is, but we can avoid it by calling putIfAbsent after we set up a collector. To avoid unnecessary collector setup, we can firstly check the concurrent hash map, and only perform this operation if a collector is not present for this application id. Not sure if that makes sense... Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14511970#comment-14511970 ] Sangjin Lee commented on YARN-3390: --- Thanks [~gtCarrera9] for reminding me of this. You mentioned this in another JIRA but I couldn't find it. Then v.3 does have the same issue in that collector.init() and collector.start() are done after the lock is released. Then I think we should go back to the previous version on this (collector.init() and collector.start() should be done inside the synchronization block so that no other thread can start using it before those operations are done). Could we make that one revision? Then I think we're good to go. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14511978#comment-14511978 ] Li Lu commented on YARN-3390: - Agree with [~sjlee0] that we may want to go back to the previous version for this (neither did I find that JIRA... ). Yes, let's address this in a separate JIRA. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14512101#comment-14512101 ] Zhijie Shen commented on YARN-3390: --- [~sjlee0], congrats on your first commit! Thanks for review, Naga and Li! Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Fix For: YARN-2928 Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch, YARN-3390.4.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14512059#comment-14512059 ] Li Lu commented on YARN-3390: - Patch LGTM, thanks [~zjshen]! Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Fix For: YARN-2928 Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch, YARN-3390.4.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14511990#comment-14511990 ] Zhijie Shen commented on YARN-3390: --- So are you okay to reuse v2 patch, which is the previous version? Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[
https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14511856#comment-14511856
]
Sangjin Lee commented on YARN-3390:
---
I'm very sorry for revisiting this one more time after saying LGTM. :) But it
occurred to me while looking at your patch.
You brought up a good point that collector.init() and collector.start() can be
done without synchronization after put() is done (based on the collectorIsNew
flag). I now realize that it is exactly the same semantics as
ConcurrentHashMap, and we should be able to do this much more simply using
ConcurrentHashMap. Sorry for the late change, but I propose this:
{code}
private final ConcurrentMapApplicationId,TimelineCollector collectors = new
ConcurrentHashMap();
...
public TimelineCollector putIfAbsent(ApplicationId appId, TimelineCollector
collector) {
TimelineCollector collectorInTable = collectors.putIfAbsent(appId, collector);
if (collectorInTable == null) {
LOG.info(the collector for + appId + was added);
collectorInTable = collector;
// initialize, start, and add it to the collection so it can be
// cleaned up when the parent shuts down
collector.init(getConfig());
collector.start();
postPut(appId, collectorInTable);
} else {
LOG.info(the collector for + appId + already exists!);
}
return collectorInTable;
}
{code}
Other code remains the same. Essentially the synchronization block in the
current patch collapses into a single call on CHM.putIfAbsent().
Previously, either [~djp] or [~gtCarrera9] pointed out that there may be a race
condition in terms of getting different collectors if CHM is used, but looking
at this, I don't see that happening. Thoughts?
Reuse TimelineCollectorManager for RM
-
Key: YARN-3390
URL: https://issues.apache.org/jira/browse/YARN-3390
Project: Hadoop YARN
Issue Type: Sub-task
Components: timelineserver
Reporter: Zhijie Shen
Assignee: Zhijie Shen
Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch
RMTimelineCollector should have the context info of each app whose entity
has been put
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14509962#comment-14509962 ] Zhijie Shen commented on YARN-3390: --- Thanks for the comments. I've addressed Sangjin and Li's comments except: bq. maybe we'd like to mark it as unstable? It's not the API for the users, hence it's okay to leave it unannotated. bq. In TimelineCollectorWebService, why we're removing the utility function getCollector? After the refactoring, we don't need to convert appId to string. It's not necessary to wrap a single statement in a method. In addition, I changed to use hook in TimelineCollectorManager, but postRemove is called before stopping the collector, because once the collector is stopped, the hook may not be able to do something with the stopped collector. Moreover, I moved RMApp.stopTimelineCollector into FinalTransition. Suppose the collector only collects application lifecycle events, it doesn't need to stay after the app is finished. We can adjust it later if we find the collector needs to stay after the app is finished. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14510053#comment-14510053 ] Sangjin Lee commented on YARN-3390: --- [~zjshen], you might want to check out [~djp]'s comments and my response in the other JIRA here: https://issues.apache.org/jira/browse/MAPREDUCE-6335?focusedCommentId=14508378page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14508378 I think those were small but useful changes. See this patch for the changes: https://issues.apache.org/jira/secure/attachment/12727521/YARN-3437.004.patch It would be good to preserve those changes. Thanks! Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch, YARN-3390.2.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[
https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14510044#comment-14510044
]
Li Lu commented on YARN-3390:
-
Hi [~zjshen], the latest patch looks good to me. Just one note is that we need
to be careful when overriding the {{postRemove}} method. Since it's in a
synchronized block, having a long postRemove method may significantly blocks
the critical section.
I'm OK with all the rationales about the unfixed comments.
Reuse TimelineCollectorManager for RM
-
Key: YARN-3390
URL: https://issues.apache.org/jira/browse/YARN-3390
Project: Hadoop YARN
Issue Type: Sub-task
Components: timelineserver
Reporter: Zhijie Shen
Assignee: Zhijie Shen
Attachments: YARN-3390.1.patch, YARN-3390.2.patch
RMTimelineCollector should have the context info of each app whose entity
has been put
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[
https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14508097#comment-14508097
]
Li Lu commented on YARN-3390:
-
Hi [~zjshen], thanks for the patch! Here are some of my comments. Most of them
are quite minor:
# Changes in RMContainerAllocator.java appears to be irrelevant. Seems like
this is changed by an IDE by mistake (on a refactoring)?
# In the following lines
{code}
+for (String tag : app.getApplicationTags()) {
+ String value = null;
+ if ((value = getFlowContext(TimelineUtils.FLOW_NAME_TAG_PREFIX, tag)) !=
null
the first null assignment to value is marked as redundant
+ if ((value = getFlowContext(TimelineUtils.FLOW_NAME_TAG_PREFIX, tag)) !=
null
+ !value.isEmpty()) {
+ collector.getTimelineEntityContext().setFlowName(value);
+ } else if ((value =
getFlowContext(TimelineUtils.FLOW_VERSION_TAG_PREFIX, tag)) != null
+ !value.isEmpty()) {
+collector.getTimelineEntityContext().setFlowVersion(value);
+ } else if ((value = getFlowContext(TimelineUtils.FLOW_RUN_ID_TAG_PREFIX,
tag)) != null
+ !value.isEmpty()) {
+collector.getTimelineEntityContext().setFlowRunId(Long.valueOf(value));
+ }
{code}
Maybe we’d like to use a switch statement to deal with this? We may first split
the tag into two parts, based on the first “:”, and then switch the first part
of the returned array to set the second part of the array into flow name,
version, and run id. Am I missing any fundamental obstacles for us to do this
here? (String switch is available from Java 7)
# Rename {{MyNMTimelineCollectorManager}} in
TestTimelineServiceClientIntegration with something indicating it's for testing?
# In the following lines:
{code}
- protected TimelineCollectorContext getTimelineEntityContext() {
+ public TimelineCollectorContext getTimelineEntityContext() {
{code}
We're exposing TimelineCollectorContext but we're not annotating the class.
Even though we may treat unannotated classes as Audience.Private, maybe we'd
like to mark it as unstable?
# In TimelineCollectorManager, I'm still having this question, although we may
not want to address it in this JIRA: are there any special consistency
requirements that prevent us from using ConcurrentHashMap?
# In TimelineCollectorWebService, why we're removing the utility function
{{getCollector}}? I think we can reuse it when adding new web services.
Reuse TimelineCollectorManager for RM
-
Key: YARN-3390
URL: https://issues.apache.org/jira/browse/YARN-3390
Project: Hadoop YARN
Issue Type: Sub-task
Components: timelineserver
Reporter: Zhijie Shen
Assignee: Zhijie Shen
Attachments: YARN-3390.1.patch
RMTimelineCollector should have the context info of each app whose entity
has been put
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14508121#comment-14508121 ] Sangjin Lee commented on YARN-3390: --- bq. In TimelineCollectorManager, I'm still having this question, although we may not want to address it in this JIRA: are there any special consistency requirements that prevent us from using ConcurrentHashMap? I can answer this as I added that code. :) In putIfAbsent(), it needs to start the collector as well if get() returns null. If we used ConcurrentHashMap and removed synchronization, multiple threads could start their own collectors unnecessarily. It is probably not a show stopper but less than desirable. Also, in real life the contention on TimelineCollectorManager is low enough that synchronization should be perfectly adequate. If we want to do this without synchronization, then we would want to use something like guava's LoadingCache. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14508136#comment-14508136 ] Li Lu commented on YARN-3390: - Hi [~sjlee0], thanks for the note! Your rational sounds quite reasonable so let keep the current design here. For now I'm OK with the coarse-grained synchronization. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14499245#comment-14499245 ] Zhijie Shen commented on YARN-3390: --- The only conflict part between YARN-3437 and this Jira is TimelineCollectorManager base class. And we happened to resort to the similar refactoring method. I'm okay to commit YARN-3437 first. However, the comments about the base TimelineCollectorManager also apply. At least, I think we should use ApplicationId instead of String. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14500062#comment-14500062 ] Sangjin Lee commented on YARN-3390: --- Thanks Zhijie. I'll move forward with the existing patch for YARN-3437. You can still make the change of String = ApplicationId as part of this JIRA (as it involves more refactoring). How's that sound? Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14500152#comment-14500152 ] Zhijie Shen commented on YARN-3390: --- bq. How's that sound? Sure, I'll take care of it. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14499039#comment-14499039 ] Li Lu commented on YARN-3390: - I skimmed through both patches. The two patches got significant overlap. Personally I'd incline to focus on YARN-3437 for now since it's critical to writer performance benchmark, which will further block the writer implementations. Writer implementation is on our critical path now to deliver an end-to-end preview of timeline v2. Therefore I'd prefer to give 3437 higher priority for now For the code, are there any special reasons why ConcurrentHashMap's fine-grained locking is not sufficient for collector manager? I think it may always be attractive to use finer-grained locking if we do not need strong consistency semantics, like snapshot or iterations. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[
https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14496511#comment-14496511
]
Sangjin Lee commented on YARN-3390:
---
{quote}
For putIfAbsent and remove, I don't use template method pattern, but let the
subclass override the super class method and invoke it inside the override
implementation, because I'm not sure if we will need pre process or post
process, and if we only invoke the process when adding a new collector. If
we're sure about template, I'm okay with the template pattern too.
{quote}
I'm fine with either approach. The main reason I thought of that is I wanted to
be clear that the base implementation of putIfAbsent() and remove() is
mandatory (i.e. not optional). Since we control all of it (base and
subclasses), it might not be such a big deal either way.
Reuse TimelineCollectorManager for RM
-
Key: YARN-3390
URL: https://issues.apache.org/jira/browse/YARN-3390
Project: Hadoop YARN
Issue Type: Sub-task
Components: timelineserver
Reporter: Zhijie Shen
Assignee: Zhijie Shen
Attachments: YARN-3390.1.patch
RMTimelineCollector should have the context info of each app whose entity
has been put
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14497341#comment-14497341 ] Sangjin Lee commented on YARN-3390: --- I took a pass at the patch, and it looks good for the most part. I would ask you to reconcile the TimelineCollectorManager changes with what I have over on YARN-3437. Again, I have a slight preference for the hook/template methods for the aforementioned reason, but it's not a strong preference one way or another. However, I'm not sure why there is a change for RMContainerAllocator.java. It doesn't look like an intended change? Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14495615#comment-14495615 ] Sangjin Lee commented on YARN-3390: --- Thanks [~Naganarasimha] and [~zjshen]! I'll take a look at the patch a little more closely later. I just wanted to bring to your attention YARN-3437. There I did a fairly similar (but not identical) refactoring of TimelineCollectorManager as part of isolating the core piece of the timeline collector manager independent of the NM-TS interaction, etc. So hopefully we can arrive at a version that can satisfy both needs. Also, a small nit: I'm not too sure if I like the name NMTimelineCollectorManager (the NM part of it). It suggests bit too strongly that this is part of the NM. How about NodeTimelineCollectorManager or another bit more independent name? I can't think of many other alternatives, so any suggestion is welcome. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14495169#comment-14495169 ] Li Lu commented on YARN-3390: - I think for the problem of dependencies, our current project structure is too coarse-grained. Ideally we may want to have something like timeline-client for NM/RM/AMs to depend on. For now, the plan to reuse most of the collector's code LGTM. We can put app-collector mapping into the RM collector manager, just as storage layer connections. Chopping down dependencies in the future would be much easier than removing (mostly but not completely) duplicated collector code IMHO. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14495645#comment-14495645 ] Zhijie Shen commented on YARN-3390: --- bq. There I did a fairly similar (but not identical) refactoring of TimelineCollectorManager as part of isolating the core piece of the timeline collector manager independent of the NM-TS interaction, etc. So hopefully we can arrive at a version that can satisfy both needs. Interesting! It seems that we both find most of the code of TimelineCollectorManager in different places. I have quick glance and find two difference: 1. I change appId type from string to ApplicationId, because I think the strong type, ApplicationId, is more commonly used in YARN, while I see we have trivial conversion from string to ApplicationId and then to string again. 2. For putIfAbsent and remove, I don't use template method pattern, but let the subclass override the super class method and invoke it inside the override implementation, because I'm not sure if we will need pre process or post process, and if we only invoke the process when adding a new collector. If we're sure about template, I'm okay with the template pattern too. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[ https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14495083#comment-14495083 ] Zhijie Shen commented on YARN-3390: --- To help folks understand the patch, I introduce a bit more about the skeleton of the code change: * TimelineCollectorManager: the base class of managing collectors' life cycles ** NMTimelineCollectorManager: extends TimelineCollectorManager, start web server to receive the incoming request and start the RPC channel for getting the context info publishing the service address. ** RMTimelineCollectorManager: extends TimelineCollectorManager, and source rmContext for getting the context info. * ResourceManager: create RMTimelineCollectorManager and make it accessible inside the daemon via RMContext. * RMAppManager: add and remove app-level collector into/out of RMTimelineCollectorManager according to RMApp's lifecycle. After this, YARN-3044's scope is clearer, inside SystemMetricsPublisher, if we're using YTS v2, we compose the new timeline entity, pick the corresponding app-level collector from RMTimelineCollectorManager, and write through it. It's similar to what we're doing for writing DS and MR data to new timeline service. Reuse TimelineCollectorManager for RM - Key: YARN-3390 URL: https://issues.apache.org/jira/browse/YARN-3390 Project: Hadoop YARN Issue Type: Sub-task Components: timelineserver Reporter: Zhijie Shen Assignee: Zhijie Shen Attachments: YARN-3390.1.patch RMTimelineCollector should have the context info of each app whose entity has been put -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (YARN-3390) Reuse TimelineCollectorManager for RM
[
https://issues.apache.org/jira/browse/YARN-3390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14495151#comment-14495151
]
Naganarasimha G R commented on YARN-3390:
-
Thanks for the patch [~zjshen], As earlier mentioned in Yarn-3044 if we go
ahead with the notion that RM will be aware of RMCollector(manager) through
RMcontext then in future if we want to segregate the dependencies b/w RM and
Timeline service projects again need to do the changes in the core part
(RMAppImpl and Resourcemanager), If the dependencies are fine then approach
looks fine to me .
Alternative way to avoid dependency is invoking RMTimelineCollectorManager's
{{putIfAbsent()}} and {{remove(ApplicationId appId)}} when SMP needs to
publish publishApplicationCreatedEvent and publishApplicationFinishedEvent. In
the approach what i have taken in Yarn-3044, RMTimelineCollector(can be renamed
to TimelineServiceV2Publisher which will be responsible for publishing v2
events) can invoke RMTimelineCollectorManager's {{putIfAbsent()}} and
{{remove(ApplicationId appId)}}.
Reuse TimelineCollectorManager for RM
-
Key: YARN-3390
URL: https://issues.apache.org/jira/browse/YARN-3390
Project: Hadoop YARN
Issue Type: Sub-task
Components: timelineserver
Reporter: Zhijie Shen
Assignee: Zhijie Shen
Attachments: YARN-3390.1.patch
RMTimelineCollector should have the context info of each app whose entity
has been put
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
