[jira] [Comment Edited] (YARN-5124) Modify AMRMClient to set the ExecutionType in the ResourceRequest
[
https://issues.apache.org/jira/browse/YARN-5124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15317575#comment-15317575
]
Arun Suresh edited comment on YARN-5124 at 6/7/16 12:48 AM:
Uploading patch addressing most of [~curino]'s feedback :
bq. Can we wrap this datastructure in some object? Map>>> it is starting to
get a bit too complicated to follow, and we might eventually want to change
representations.
I agree, it looks ugly, but even if we pull it up in a new class, we would
still see it. In the latest patch I alleviated the issue by somewhat emulating
C++ typedefs as follows:
# I create *typed* subclasses of HashMap classes like so:
{noformat}
class CapabilityMap extends TreeMap {}
class ExecutionTypeMap extends HashMap {}
class LocationMap extends HashMap {}
class RemoteRequestsTable extends HashMap {}
{noformat}
# The remoteRequestsTable then looks like this:
{noformat}
final RemoteRequestsTable remoteRequestsTable = new RemoteRequestsTable();
{noformat}
bq. Do you need the new constructor AMRMClientImpl(ApplicationMasterProtocol
protocol)? Can't you use mockito for the conf in tests and pass the
ApplicationMasterProtocol that way?
I can't use mockito, since I actually have a real AMRMClientImpl object. This
is a functional test that uses an actual MiniYARNCluster. So don't think there
is any other way to inject a custom ApplicationMasterProtocol. But I did add a
{{@VisibleForTesting}} as per [~kasha]'s suggestion.
bq. The ordering of where ExecutionType goes is not consistent, in
ResourceRequest is at the end, while in most other places is earlier. Strong
typing make this not a big deal, just looks cleaner if consistent.
The reason I had to put ExecutionType in the end for {{ContainerRequest}} and
{{ResourceRequest}} was that otherwise, would have to change many of the
methods marked {{@Stable}} in previous releases.. In the AMRMClientImpl, it
appears before the {{Resource}} in the composite key for reasons i mentioend
[here|https://issues.apache.org/jira/browse/YARN-5124?focusedCommentId=15303099&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15303099]
but thats an implementation detail and hidden from the user.
Hope this made sense..
was (Author: asuresh):
Uploading patch addressing most of [~curino]'s feedback :
bq. Can we wrap this datastructure in some object? Map>>> it is starting to
get a bit too complicated to follow, and we might eventually want to change
representations.
I agree, it looks ugly, but even if we pull it up in a new class, we would
still see it. In the latest patch I alleviated the issue by somewhat emulating
C++ typedefs as follows:
# I create *typed* subclasses of HashMap classes like so:
{noformat}
class CapabilityMap extends TreeMap {}
class ExecutionTypeMap extends HashMap {}
class LocationMap extends HashMap {}
class RemoteRequestsTable extends HashMap {}
{noformat}
# The remoteRequestsTable then looks like this:
{noformat}
final RemoteRequestsTable remoteRequestsTable = new RemoteRequestsTable();
{noformat}
bq. Do you need the new constructor AMRMClientImpl(ApplicationMasterProtocol
protocol)? Can't you use mockito for the conf in tests and pass the
ApplicationMasterProtocol that way?
I can't use mockito, since I actually have a real AMRMClientImpl object. This
is a functional test that uses an actual MiniYARNCluster. So don't think there
is any other way to inject a custom ApplicationMasterProtocol. But I did add a
{{@VisibleForTesting}} as per [~kasha]'s suggestion.
bq. The ordering of where ExecutionType goes is not consistent, in
ResourceRequest is at the end, while in most other places is earlier. Strong
typing make this not a big deal, just looks cleaner if consistent.
The reason I had to put ExecutionType in the end for {{ContainerRequest}} and
{{ResourceRequest}} was that otherwise, would have to change many of the
methods marked {{@Stable}} in previous releases.. In the AMRMClientImpl, it
appears before the {{Resource}} in the composite key for reasons i mentioend
[here|https://issues.apache.org/jira/browse/YARN-5124?focusedCommentId=15303099&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15303099]
but thats an implementation detail and hidden from the user.
Hope this made sense..
> Modify AMRMClient to set the ExecutionType in the ResourceRequest
> -
>
> Key: YARN-5124
> URL: https://issues.apache.org/jira/browse/YARN-5124
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Arun Suresh
>Assignee: Arun Suresh
> Attachments: YARN-5124.001.patch, YARN-5124.002.patch,
> YARN-5124.003.patch, YARN-5124.004.patch, YARN-5124.005.patch,
> YARN-5124.006.patch, YARN-5124
[jira] [Comment Edited] (YARN-5124) Modify AMRMClient to set the ExecutionType in the ResourceRequest
[
https://issues.apache.org/jira/browse/YARN-5124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15317575#comment-15317575
]
Arun Suresh edited comment on YARN-5124 at 6/7/16 12:47 AM:
Uploading patch addressing most of [~curino]'s feedback :
bq. Can we wrap this datastructure in some object? Map>>> it is starting to
get a bit too complicated to follow, and we might eventually want to change
representations.
I agree, it looks ugly, but even if we pull it up in a new class, we would
still see it. In the latest patch I alleviated the issue by somewhat emulating
C++ typedefs as follows:
# I create *typed* subclasses of HashMap classes like so:
{noformat}
class CapabilityMap extends TreeMap {}
class ExecutionTypeMap extends HashMap {}
class LocationMap extends HashMap {}
class RemoteRequestsTable extends HashMap {}
{noformat}
# The remoteRequestsTable then looks like this:
{noformat}
final RemoteRequestsTable remoteRequestsTable = new RemoteRequestsTable();
{noformat}
bq. Do you need the new constructor AMRMClientImpl(ApplicationMasterProtocol
protocol)? Can't you use mockito for the conf in tests and pass the
ApplicationMasterProtocol that way?
I can't use mockito, since I actually have a real AMRMClientImpl object. This
is a functional test that uses an actual MiniYARNCluster. So don't think there
is any other way to inject a custom ApplicationMasterProtocol. But I did add a
{{@VisibleForTesting}} as per [~kasha]'s suggestion.
bq. The ordering of where ExecutionType goes is not consistent, in
ResourceRequest is at the end, while in most other places is earlier. Strong
typing make this not a big deal, just looks cleaner if consistent.
The reason I had to put ExecutionType in the end for {{ContainerRequest}} and
{{ResourceRequest}} was that otherwise, would have to change many of the
methods marked {{@Stable}} in previous releases.. In the AMRMClientImpl, it
appears before the {{Resource}} in the composite key for reasons i mentioend
[here|https://issues.apache.org/jira/browse/YARN-5124?focusedCommentId=15303099&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15303099]
but thats an implementation detail and hidden from the user.
Hope this made sense..
was (Author: asuresh):
Uploading patch addressing most of [~curino]'s feedback :
bq. Can we wrap this datastructure in some object? Map>>> it is starting to
get a bit too complicated to follow, and we might eventually want to change
representations.
I agree, it looks ugly, but even if we pull it up in a new class, we would
still see it. In the latest patch I alleviated the issue by somewhat emulating
C++ typedefs as follows:
# I create *typed* subclasses of HashMap classes like so:
{noformat}
class CapabilityMap extends TreeMap {}
class ExecutionTypeMap extends HashMap {}
class LocationMap extends HashMap {}
class RemoteRequestsTable extends HashMap {}
{noformat}
# The remoteRequestsTable then looks like this:
{noformat}
final RemoteRequestsTable remoteRequestsTable = new RemoteRequestsTable();
{noformat}
bq. Do you need the new constructor AMRMClientImpl(ApplicationMasterProtocol
protocol)? Can't you use mockito for the conf in tests and pass the
ApplicationMasterProtocol that way?
I can't use mockito, since I actually have a real AMRMClientImpl object. This
is a functional test that uses an actual MiniYARNCluster. So don't think there
is any other way to inject a custom ApplicationMasterProtocol. But I did add a
{{@VisibleForTesting}} as per [~kasha]'s suggestion.
bq. The ordering of where ExecutionType goes is not consistent, in
ResourceRequest is at the end, while in most other places is earlier. Strong
typing make this not a big deal, just looks cleaner if consistent.
The reason I had to put ExecutionType in the end for {{ContainerRequest}} and
{{ResourceRequest}} was that otherwise, would have to change many of the
methods marked {{@Stable}} in previous releases.. In the AMRMClientImpl, it
appears before the {{Resource}} in the composite key for reasons i mentioend
[here|https://issues.apache.org/jira/browse/YARN-5124?focusedCommentId=15303099&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15303099]
but thats an implementation detail and hidden from the user.
Hope this made sense..
> Modify AMRMClient to set the ExecutionType in the ResourceRequest
> -
>
> Key: YARN-5124
> URL: https://issues.apache.org/jira/browse/YARN-5124
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Arun Suresh
>Assignee: Arun Suresh
> Attachments: YARN-5124.001.patch, YARN-5124.002.patch,
> YARN-5124.003.patch, YARN-5124.004.patch, YARN-5124.005.patch,
> YARN-5124.006.patch, YARN-5124.008.patch,
[jira] [Comment Edited] (YARN-5124) Modify AMRMClient to set the ExecutionType in the ResourceRequest
[
https://issues.apache.org/jira/browse/YARN-5124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15316147#comment-15316147
]
Arun Suresh edited comment on YARN-5124 at 6/6/16 6:00 AM:
---
[~kasha], thanks for the review..
I agree with 1-4 of your comments.. shall upload patch addressing them shortly..
w.r.t #5,
Given that {{ExecutionTypeRequest}} is a wrapper for the actual
{{ExecutionType}}, the remoteRequestsTable primary key should comprise the
ExecutionType, not the ExecutionTypeRequest. I was thinking, for the case where
enforceExecutionType is false, we can possibly just match a returned Container
with any entry in the remoteRequestsTable (where Priority, Location and
Capability match, but the ExecutionType can be anything).
I intentionally left out the implementation, since currently, only Distributed
Scheduling supports ExecutionTypes and it currently only supports
enforceExecutionType = true. I would be happy to put at doc with a TODO there
(and maybe open a JIRA) to fix it once we have the Scheduler fix that supports
enforceExecutionType = false.
Thoughts ?
was (Author: asuresh):
[~kasha], thanks for the review..
I agree with 1-4 of your comments.. shall upload patch addressing them shortly..
w.r.t #5,
Given that {{ExecutionTypeRequest}} is a wrapper for the actual
{{ExecutionType}}, the remoteRequestsTable primary key should comprise the
ExecutionType, not the ExecutionTypeRequest. I was thinking, for the case where
enforceExecutionType is false, we can possibly just match a returned Container
with any entry in the remoteRequestsTable (where Priority, Location and
Capability match, but the ExecutionType can be anything).
I intentionally left out the implementation, since currently, the only
Distributed Scheduling supports ExecutionTypes and it currently only supports
enforceExecutionType = true. I would be happy to put at doc with a TODO there
(and maybe open a JIRA) to fix it once we have the Scheduler fix that supports
enforceExecutionType = false.
Thoughts ?
> Modify AMRMClient to set the ExecutionType in the ResourceRequest
> -
>
> Key: YARN-5124
> URL: https://issues.apache.org/jira/browse/YARN-5124
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Arun Suresh
>Assignee: Arun Suresh
> Attachments: YARN-5124.001.patch, YARN-5124.002.patch,
> YARN-5124.003.patch, YARN-5124.004.patch, YARN-5124.005.patch,
> YARN-5124.006.patch, YARN-5124.008.patch,
> YARN-5124_YARN-5180_combined.007.patch, YARN-5124_YARN-5180_combined.008.patch
>
>
> Currently the {{ContainerRequest}} allows the AM to set the {{ExecutionType}}
> in the AMRMClient, but it is not being set in the actual {{ResourceRequest}}
> that is sent to the RM
--
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
