[
https://issues.apache.org/jira/browse/YARN-2209?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14180868#comment-14180868
]
Zhijie Shen commented on YARN-2209:
-----------------------------------
bq. Given that we already broken compatibility for rolling upgrades, the patch
should be fine in that sense.
Hm... Make sense. The patch is good overall. Just some minor comments about it.
1. Use ApplicationAttemptNotFoundException instead of
ApplicationNotFoundException?
2. Node need to suppress the each method if you already suppress the class.
{code}
@SuppressWarnings("deprecation")
public synchronized AMCommand getAMCommand() {
{code}
3. Is the change in ResourceCalculator.java related?
4. I saw deprecation warning around "import
org.apache.hadoop.yarn.api.records.AMCommand;" in TestAllocateResponse. Is this
the reason for jenkins -1 javac?
5. *H*eartbeats, to be consistent with other constant strings.
{code}
public static final String AM_ALLOCATE = "App Master heartbeats";
{code}
6. Instead of "Assert.assertTrue(catchException);", why not "Assert.fail()"
directly after "ar = am.allocate("h1", 1000, request, new
ArrayList<ContainerId>());" Same for the following assertions.
{code}
boolean catchException = false;
try {
ar = am.allocate("h1", 1000, request, new ArrayList<ContainerId>());
} catch (ApplicationMasterNotRegisteredException e) {
catchException = true;
}
Assert.assertTrue(catchException);
{code}
7. Unused import in TestAMRMRPCResponseId.java
{code}
Throwable exception = null;
try {
allocate(attempt.getAppAttemptId(), allocateRequest);
} catch (Exception e) {
exception = e.getCause();
}
Assert
.assertTrue(exception instanceof
InvalidApplicationMasterRequestException);
{code}
can be changed to
{code}
try {
allocate(attempt.getAppAttemptId(), allocateRequest);
Assert.fail()
} catch (Exception e) {
Assert.assertTrue(e instanceof InvalidApplicationMasterRequestException);
}
{code}
8. No need to change to split a single statement into the following two.
{code}
allocateResponse = allocate(progressIndicator);
return allocateResponse;
{code}
9. Unnecessary import in AMRMClientAsyncImpl.java
10. After the following change, the method is going to return early in resync
case. Is it expected. Why does it not need to take the remaining operations
after code change?
{code}
if (response.getAMCommand() != null) {
switch(response.getAMCommand()) {
case AM_RESYNC:
LOG.info("ApplicationMaster is out of sync with ResourceManager,"
+ " hence resyncing.");
lastResponseID = 0;
// Registering to allow RM to discover an active AM for this
// application
register();
addOutstandingRequestOnResync();
break
{code}
to
{code}
} catch (ApplicationMasterNotRegisteredException e) {
LOG.info("ApplicationMaster is out of sync with ResourceManager,"
+ " hence resync and send outstanding requests.");
// RM may have restarted, re-register with RM.
lastResponseID = 0;
register();
addOutstandingRequestOnResync();
return null;
{code}
At last, would you mind filing a separate MR Jira, and splitting the patch into
two for committing?
> Replace AM resync/shutdown command with corresponding exceptions
> ----------------------------------------------------------------
>
> Key: YARN-2209
> URL: https://issues.apache.org/jira/browse/YARN-2209
> Project: Hadoop YARN
> Issue Type: Improvement
> Reporter: Jian He
> Assignee: Jian He
> Attachments: YARN-2209.1.patch, YARN-2209.2.patch, YARN-2209.3.patch,
> YARN-2209.4.patch, YARN-2209.5.patch, YARN-2209.6.patch, YARN-2209.6.patch
>
>
> YARN-1365 introduced an ApplicationMasterNotRegisteredException to indicate
> application to re-register on RM restart. we should do the same for
> AMS#allocate call also.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
