Rohith Sharma K S,
This i had tried to do mimic same as in ATSv1 so that the users of the ATS v1 are not impacted. Any particular reason to move it to info level ?

I think the major concern is that when putting useful fields into event info field instead of entity info field, we lost the ability to easily query them. For example, if an user would like to list all containers with a specific exit code, or finished within a time window. This said, I think the key point is to put data related to the whole entity into the entity's info field. Ideally we should only put some quite lightweight data into the event's info fields (since they're not easily to query), but if we share a lot of code path we may want to either replicate those data in entity info, or refactor the code?

When I raised JIRA, my concern was too as Li mentioned. We have lost ability to easily query yarn entities by adding few information in event info fields.

Currently, ATS does not support querying for event-info filter. And even if ATS supports, it is again a burden for user to parse the information from different events. For example,YARN container has information 3 different places. Firstly, info field of entity about user, host detail, priority etc. Secondly, event RM_CONTAINER_CREATED holds information about container start time and other. Thirdly, event RM_CONTAINER_CREATED holds diagnostics, state and status.

So, I think entity related information better to publish at entity info level. It is basically refactoring code.

For containers, some of the event info should be published at container info level.

Agree with the intention behind this JIRA Rohith Sharma K S . Some info which we may have to query and does not change per event i.e. this info is not repeated across events, we should have at entity info level. It will enable users to use info filters for it.
Only info which is strictly specific to an event is what can be kept at event info level. This we can keep as the thumb rule for publishing info. However, we can also consider publishing it at both places on need basis (i.e. if and when we find a use case).

Thanks Rohith Sharma K S & Li Lu for detailing it out.

For example : container exist status, container state, createdTime, finished time.

Pushing these kind of information both the places would be fine but IMO diagnostics kind of stuff need not be stored. Both the places to ensure existing clients need not be impacted.

Thirdly, event RM_CONTAINER_CREATED holds diagnostics, state and status.

IIUC you mean container finished right ?

In between (not related to this jira ) One more thing i was noticing was how will user know what are the different entity types present which needs to be queried, do we need to capture in our documentation ? For example MR entities, what are the different entity ID's to query ? Where are the MR Job configs present? how to access the MR job counters?
Similarly for YARN system entities to query the resource usage and the containers stats(number of containers for different priority, time taken to finish etc ...)
These were some of the queries which i have encountered from others...

I think this is the time to decide should we support translation layer for YARN entities to get attempts-report, container-reports, application-report. It can be additional web service in ATS2 similar to /ws/v1/applicationhistory.

Major concern of this JIRA is about deserializing attempt-report and container-report for Web UI. If we get much easier translation layer from java, it would be much easier. Other wise, it is tedious task for deserializing entity payload if any change in the publishing entity.

Updated the patch with following changes.
Application :

1. YARN_APPLICATION_FINISHED event has some app level information, I have moved to entity level info.

ApplicationAttempt :

1. Events YARN_APPLICATION_ATTEMPT_FINISHED and YARN_APPLICATION_ATTEMPT_REGISTERED has application report specific info details. These have been moved to entity info level.

Container :

1. Events YARN_RM_CONTAINER_CREATED and YARN_RM_CONTAINER_FINISHED had container report specific info details. These have been moved to entity info level.
2. Remove duplicated information which was published from NM publisher info.
3. Added container created and container finished information in entity level info.

-1 overall

This message was automatically generated.

updating same patch to branch YARN-5355

Rohith Sharma K S, thanks for the patch.
In the patch I see you have removed container related info from NMTimelinePublisher. Is this intentional ?
Container event publishing from RM is optional. And will be ideally be switched off due to the volume and impact on RM.

Is this intentional ?

yes, because the same information also published by RM. So I think we can keep at one place.

[
  {
    "events": [
      {
        "id": "YARN_RM_CONTAINER_FINISHED",
        "timestamp": 1475469588288,
        "info": {
          "YARN_CONTAINER_STATE": "COMPLETE",
          "YARN_CONTAINER_EXIT_STATUS": 0,
          "YARN_CONTAINER_DIAGNOSTICS_INFO": ""
        }
      },
      {
        "id": "YARN_RM_CONTAINER_CREATED",
        "timestamp": 1475469570721,
        "info": {
          "YARN_CONTAINER_ALLOCATED_PORT": 25006,
          "YARN_CONTAINER_ALLOCATED_MEMORY": 1024,
          "YARN_CONTAINER_ALLOCATED_PRIORITY": 0,
          "YARN_CONTAINER_ALLOCATED_HOST_HTTP_ADDRESS": "http://ctr-e29-1471411959733-0412-01-000004.hwx.site:25008",
          "YARN_CONTAINER_ALLOCATED_HOST": "ctr-e29-1471411959733-0412-01-000004.hwx.site",
          "YARN_CONTAINER_ALLOCATED_VCORE": 1
        }
      },
      {
        "id": "YARN_CONTAINER_CREATED",
        "timestamp": -1,
        "info": {}
      },
      {
        "id": "YARN_CONTAINER_FINISHED",
        "timestamp": -1,
        "info": {
          "YARN_CONTAINER_STATE": "RUNNING",
          "YARN_CONTAINER_EXIT_STATUS": 0,
          "YARN_CONTAINER_DIAGNOSTICS_INFO": ""
        }
      },
      {
        "id": "YARN_NM_CONTAINER_LOCALIZATION_FINISHED",
        "timestamp": -1,
        "info": {}
      },
      {
        "id": "YARN_NM_CONTAINER_LOCALIZATION_STARTED",
        "timestamp": -1,
        "info": {}
      }
    ],
    "type": "YARN_CONTAINER",
    "id": "container_e09_1475277121920_0010_01_000001",
    "createdtime": -1,
    "info": {
      "YARN_CONTAINER_ALLOCATED_PORT": 25006,
      "UID": "yarn-cluster!application_1475277121920_0010!YARN_CONTAINER!container_e09_1475277121920_0010_01_000001",
      "YARN_CONTAINER_ALLOCATED_MEMORY": 1024,
      "SYSTEM_INFO_PARENT_ENTITY": {
        "type": "YARN_APPLICATION_ATTEMPT",
        "id": "appattempt_1475277121920_0010_000001"
      },
      "YARN_CONTAINER_ALLOCATED_PRIORITY": "0",
      "YARN_CONTAINER_ALLOCATED_HOST": "ctr-e29-1471411959733-0412-01-000004.hwx.site",
      "YARN_CONTAINER_ALLOCATED_HOST_HTTP_ADDRESS": "ctr-e29-1471411959733-0412-01-000004.hwx.site:0",
      "YARN_CONTAINER_ALLOCATED_VCORE": 1
    },
    "configs": {},
    "isrelatedto": {},
    "relatesto": {}
  }
]

Container event publishing from RM is optional.

I see. But I think we should make it mandatory to publish from RM since some information always get from RM like container_state. Otherwise we miss the this information else container_state info should be published from NM. YARN-5156 removes from NM publisher.

There was a detailed discussion on this and then we concluded it to be in NM only and yes all the required information is getting published from NM right ?

Had missed YARN-5156 let me check how to include that scenario too

Otherwise we miss the this information else container_state info should be published from NM

So the reason this was removed in YARN-5156 was that container state can either be RUNNING and COMPLETED and it becomes COMPLETED only on CONTAINER_FINISHED event. Which means it can be easily deduced from event. At the time container state was carried in event info so the majority opinion went with removing container state altogether.
I have made a comment on YARN-5156 too.

-1 overall

This message was automatically generated.

Updated patch by reverting changes made in NMTimelinePublisher.

Updated patch also fixes a another bug i.e always NM published http address port used to come as zero. This is because of when NMPublisher started, web server was not started which resulting in http port used be zero.

-1 overall

This message was automatically generated.

Thanks Rohith Sharma K S for the latest patch.
Few comments:

1. In NMTimelinePublisher#publishContainerEvent, shouldn't we check against httpAddress being null ? Instead of not being null.
2. I see that we are publishing tracking URL twice. If AHS is enabled, tracking URL is changed to AHS web endpoint when attempt finishes. So info field will have its latest value as AHS url. In that case it should be attached to event also so that we know the original AM tracking URL too (i.e. when attempt was registered). Thoughts ?
3. In TimelineServiceV2Publisher#appStateUpdated, changes are not required as we are effectively doing the same thing as before. We should definitely publish this info at event level because event is app state update event. Also should we update it at info level too so that we can filter apps while they are running based on their current state ?
4. Do we need to publish master container info at app attempt level twice ?

Updated patch also fixes a another bug i.e always NM published http address port used to come as zero.

Thanks for the fix.

For tracking URL part though, AHS will not be enabled with ATSv2 and new Web UI.

Thanks Rohith Sharma K S for the patch! Sorry it took me a while to get to this.

I generally agree with pulling some info from events to the entity level, especially if they can be part of queries. We do want to be careful not to put everything at the entity level, however. We probably want to limit it to only the things we will actually query.

In more detail,
(NMTimelinePublisher.java)

• l.182: we’re already setting the entity created time in l.190; do we need to add it to the entity info? I would say, we don't need this
• l.203: do we need to add diagnostics to the entity level? Not sure if it is useful. Can we keep it on the event?
• l.307: the same comment as Varun’s; should be the other way around?
• regarding the container state that was removed in YARN-5156; perhaps we should add it as entity info here (with COMPLETED)?

(ContainerMetricsConstants.java)

• l.44: see NMTimelinePublisher.java

(TimelineServiceV2Publisher.java)

• l.158: same as above; is diagnostics needed at the entity level?
• l.190: this should be entity.setInfo() not tEvent.setInfo()
• l.289: same as above

It would be great if we can add some unit tests to check these. Thanks!

regarding the container state that was removed in YARN-5156; perhaps we should add it as entity info here (with COMPLETED)?

Agree, we can do so. That was one of the suggested options in YARN-5156, set it to RUNNING in container start and COMPLETED on container end.

Thanks for the work Rohith Sharma K S! Generally fine but a few concerns:
1. I agree with Varun and Sangjin that we may need to think twice about diagnostic info. This field is way too cumbersome for users to directly query. I think it's fine to leave it in event info.
2. When we get app state updated event, we may want to post the event info as is, but at the same time update entity level information?
3. A bigger concern is on the code itself. Right now we have a lot of info key name constants ending as "EVENT_INFO", but actually we no longer use them as event info. Instead, we may post them as entity info, or event info, or both. We need to update or simplify those constants to make them less confusing.

To me, goal for moving event info to entity info is User should be able to get application/application-attempt/container reports at entity-info rather than entity-event-info. The main advantage is user need not iterate trough every events to get the report details. Iterating is time consuming when rendering data in web UI by deserializing the pay load.

Li Lu

A bigger concern is on the code itself. Right now we have a lot of info key name constants ending as "EVENT_INFO",

True, I do a change removing EVENT in all the fields in next patches.

Sangjin Lee

regarding the container state that was removed in YARN-5156; perhaps we should add it as entity info here (with COMPLETED)?

I will make a change adding container_state in next patches

we’re already setting the entity created time in l.190; do we need to add it to the entity info? I would say, we don't need this

It is only for easier accessibility rather than going through event time stamps.

It is only for easier accessibility rather than going through event time stamps.

Created time is at the topmost level in an entity. We don't need to go through event timestamps for it. Other intention of having created time in info would be if we want to send a query like return me all entities which have a specific created time in milliseconds. Highly unlikely somebody would want this. Even this though, can be achieved by using createdTimeStart/end filters.
To me, we are repeating this information by keeping it in info. I think it will serve your use case even otherwise. Thoughts ?

cc Rohith Sharma K S

Created time is at the topmost level in an entity. We don't need to go through event timestamps for it.

What about end time? End time has to be get from event time stamp. But anyway, I have added a comment for streamlining YARN entities publishing, I think we should decide for providing YARN entities converter rather than fixing individual many bugs.

Actually let's take one step backward... What is the key benefit for keeping most data in event info, rather than in entity info? To me, storing data in entity info map looks a good representation that the data "belongs to" the whole entity, but not just one event. Even though some data may come from a certain type of timeline event, if it adds useful information to the whole entity, we should put them in entity info field. I'm not sure if there are some implementation side concerns that prevent us from doing this. However, even if so, I believe we should fix those blockers but not solve the problem the other way around (avoid putting data in entity info, but just put them in event info). I would de-prioritize the work on making the v2 code path consistent with v1's if this is a problem for our current design.

What about end time? End time has to be get from event time stamp. But anyway, I have added a comment for streamlining YARN entities publishing, I think we should decide for providing YARN entities converter rather than fixing individual many bugs.

Created time is a first-class attribute of an entity, so it would be good to use that instead of a generic entity info field. End time is not a first-class attribute, so entity info would be a natural place.

Actually let's take one step backward... What is the key benefit for keeping most data in event info, rather than in entity info? To me, storing data in entity info map looks a good representation that the data "belongs to" the whole entity, but not just one event. Even though some data may come from a certain type of timeline event, if it adds useful information to the whole entity, we should put them in entity info field. I'm not sure if there are some implementation side concerns that prevent us from doing this. However, even if so, I believe we should fix those blockers but not solve the problem the other way around (avoid putting data in entity info, but just put them in event info). I would de-prioritize the work on making the v2 code path consistent with v1's if this is a problem for our current design.

I'm not so sure if I understand what you meant by this. Are you proposing elevating all event info items to the entity info unless they really belong only to events? Or are you proposing not to do this work? Bit confused...

Are you proposing elevating all event info items to the entity info unless they really belong only to events?

This is my intention. If the event carries information about the timeline entity, why not posting it as an entity level info?

Thanks for the clarification. Then what do you think of the currently proposed set of items in the patch? Do you think they all belong at the entity level? I think one thing we were debating was the diagnostic info.

Thanks Sangjin Lee. This said I'm generally fine with Rohith's planning. End time looks belong to an entity but not just an event, too. For start time, it looks natural to have some queries like "list all the apps start in a time window and finished in a time window"? So if it's not too expensive, I think it's fine to also put it in info field?

The created time just seems redundant. Is there something that cannot be done by the explicit created time attribute? Rohith said

It is only for easier accessibility rather than going through event time stamps.

We're already setting it to the entity created time, so you would not be going through event time stamps for the created time. So I'm not sure whether we need to store it again as part of the entity info.

OK, I don't have a strong preference on that. Once the data is available on entity level I think we're fine. The benefit for replicating it is not to make created time different to end time, but I'm fine with either ways.

So I'm not sure whether we need to store it again as part of the entity info.

I agree with Li lu. Even I don't have strong opinion to add duplicate created time with different info key, since it is available in entity level. Though created time is same entity createdTime, adding duplicate createdTime with info key as YARN_CONTAINER_CREATED_TIME and YARN_CONTAINER_FINISHED_TIME makes more user facing to understand it.

I'd like to know what would be made much easier with having the created time as entity info that is inconvenient or difficult with the explicit created time attribute. I get that it would be more symmetric with the finished time, but that alone is not a strong argument for replicating this info.

Since we're talking about container entities, this is probably the most number of entities in the storage. If we can help not add a column per object unnecessarily, I think it would be a good thing.

Updating the patch with following changes. Patch is rebased against trunk as this need to commit in trunk.

1. Added container finish time to get published on container finish event. container create time is not added, user can take this time from entity created time itself.
2. appUpdated event publish kept unmodified as per review comment.
3. Removed duplicated information getting published during attempt start and attempt end time as per Varun's review comment.
4. tracking URL and original trackingURL published in entity info level. Original tracking URL life time is same as application run time. When application is running, client need to construct URL with proxy/without proxy as it is done for rendering AppsBlock.
5. At last, all the constants are renamed to *_INFO rather than *_EVENT_INFO or *_ENTITY_INFO.

-1 overall

This message was automatically generated.

Test failure is not related to patch, i.e YARN-5679 handles it.
Checkstyle is related to patch, I will fix it and upload a new patch.
Before that I would like to hear review comments if any so that I can upload single patch.

Thanks for the update patch Rohith Sharma K S! I think it's almost there. A few comments.

(AppAttemptMetricsConstants.java)

• l.55: the name DIAGNOSTICS_INFO_INFO doesn't sound quite right; just DIAGNOSTICS_INFO?

(ContainerMetricsConstants.java)

• l.64: same as above

(TestApplicationHistoryManagerOnTimelineStore.java)

• I'm a little confused by this test; it appears that we're preparing these entities as before (i.e. all this info is at the event level). Is that intended? Don't we want to reflect the same changes in this test too? I wonder how this test is passing then (or what it's testing even)?

(NMTimelinePublisher.java)

• l.207: I suspect it might be the same, but just to be explicit, should we do toString()?

the name DIAGNOSTICS_INFO_INFO doesn't sound quite right; just DIAGNOSTICS_INFO?

make sense to me, I did not notice this. I will update this.

I'm a little confused by this test; it appears that we're preparing these entities as before (i.e. all this info is at the event level). Is that intended? Don't we want to reflect the same changes in this test too? I wonder how this test is passing then (or what it's testing even)?

frankly, I haven not done any modification in this test classes explicitly. I did *MetricsConstants classes refactoring which resulted in changes in some of the test classes. I can look at these test class and if any minor modifications I can handle it in this patch itself, otherwise we might need to re visit whole test class again that could be done in separate JIRA.

And also I notice that there is no specific test cases that are validating individual fields for entity that are published to ATSv2 in TestSystemMetricsPublisherForV2. Again it is bunch of test cases to be added to validate each fields.

OK it's more of curiosity on my part as what this test does is obviously different than what we are doing in this patch. I am fine with opening a different JIRA to look into this test.

Basically TestApplicationHistoryManagerOnTimelineStore runs on ATSv1. They publish the entities to store and read it back where they verify each fields for validating published value is read. In ATSv2, there is such framework exists as of now. I have raised a JIRA YARN-5737 to validate each fields that are published in the form of report.

Updated the patch fixing review comment and checkstyle.

-1 overall

This message was automatically generated.

Test failures are unrelated to patch. YARN-5377 and YARN-5737 are tracking JIRA for test failures.

The latest patch LGTM. I'll wait until tomorrow to give folks time to chime in before I commit.

Rohith Sharma K S, just to be clear, is the intent to commit this to trunk? If I take the latest patch (https://issues.apache.org/jira/secure/attachment/12833383/0003-YARN-5699.patch) it does not apply cleanly to the YARN-5355 branch. Could you please upload the corresponding YARN-5355 patch?

Yes, current patch given based on trunk. Sure, I will upload patch for YARN-5355 branch.

Updated patch to YARN-5355 branch

-1 overall

This message was automatically generated.

Committed it to trunk, YARN-5355 and YARN-5355-branch-2. Thanks Rohith Sharma K S for your contribution, and others for valuable reviews!

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10622 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10622/)
YARN-5699. Retrospect yarn entity fields which are publishing in events (sjlee: rev 1f304b0c7f261369dd68839507bb609a949965ad)

• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/timelineservice/NMTimelinePublisher.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/applicationhistoryservice/TestApplicationHistoryManagerOnTimelineStore.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/metrics/TestSystemMetricsPublisher.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/ApplicationHistoryManagerOnTimelineStore.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/metrics/TimelineServiceV1Publisher.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/metrics/TimelineServiceV2Publisher.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/metrics/AppAttemptMetricsConstants.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/metrics/ContainerMetricsConstants.java