[
https://issues.apache.org/jira/browse/MAPREDUCE-6838?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16134241#comment-16134241
]
Varun Saxena commented on MAPREDUCE-6838:
-----------------------------------------
Thanks [~rohithsharma] for the review.
bq. Need to log a WARN message if collector info is null.
Ok.
bq. currentTimelineToken should be volatile
Need not be. Atleast in MR AM. This is used only while AM is updating the token
and that happens only from RMContainer Allocator thread so only one thread sees
and updates it. While using token is picked from UGI. Will it be likely that
token will be updated from 2 separate threads? We anyways do not claim any
thread safety for timeline client.
Address is volatile and its different because the thread publishing the entity
and using the address would be different from the allocator thread which would
communicate with RM and update the address. However, making it volatile doesn't
cost us anything. As you say. I do not have a strong opinion on this. Thoughts?
bq. Creating Token does not required to check service==null. Internally
constructor does. And we can ignore token service passed by delegationToken
always and set it up collector address.
You mean the constructor inside setTimelineDelegationToken method i.e. at L203?
Actually the constructor takes service as Text and not String. The check I am
making is for service as String. If I do not make the check and call new
Text(service), a null service would throw NPE.
bq.
!delegationToken.getKind().equals(TimelineDelegationTokenIdentifier.KIND_NAME.toString())
check is not required since equals does this comparrission too.
Didn't quite get you. This is to avoid updating token for another kind. This is
to avoid updating a token altogether i.e. even if we do not have a previous
token. The equals check is for not updating the token if it is equal to cached
token. If I remove this check, a token of another kind will be added in UGI.
bq. In CollectorInfo object, If collector address is null and Token is
non-null. Do not add that token into ugi.
Check like this required? If token service exists and timeline service address
is already updated, should we not update the token, if we look at this piece of
code independently. Currently we send both together but the protocol doesn't
enforce it. The proto definition of CollectorInfo marks collector address field
as optional.
These checks are primarily for robustness if we consider the TimelineV2Client
code in isolation and not merely go by what we currently know RM sends. If we
make assumptions based on current implementation, we are tightly coupling the
RM/NM logic with logic here and it is not enforced by protocol either. It is
likely to work just fine as developers would take care but I would suggest that
ideally if we assume that collector address is carried always, we enforce it in
proto definition of CollectorInfo i.e. make collector address as "required"
instead of "optional" in it. And we will have to see if address should be
"required" in AppCollectorData too.
Please note that RM may not have access to collector address initially when AM
container is launched. So this change would also mean change in RM to not send
collector info at all if address is null.
The last comment i.e. suggested refactoring depends on comments above i.e.
whether to update the token or not if address is not carried in collector info.
Thoughts?
> [ATSv2 Security] Add timeline delegation token received in allocate response
> to UGI
> -----------------------------------------------------------------------------------
>
> Key: MAPREDUCE-6838
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-6838
> Project: Hadoop Map/Reduce
> Issue Type: Sub-task
> Reporter: Varun Saxena
> Assignee: Varun Saxena
> Labels: yarn-5355-merge-blocker
> Fix For: YARN-5355
>
> Attachments: MAPREDUCE-6838-YARN-5355.01.patch,
> MAPREDUCE-6838-YARN-5355.02.patch, MAPREDUCE-6838-YARN-5355.03.patch,
> MAPREDUCE-6838-YARN-5355.04.patch, MAPREDUCE-6838-YARN-5355.05.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
