Agree with Wangda Tan, we should also probably mark it as @Private

Probably mark it to private is not enough, because we should not make a public/stable class (ResourceRequest) inherit from a private/unstable class. And no matter what is the annotation of the AbstractResourceRequest itself, we have to mark get/setCapability to public/stable. I think we need to remove the class itself and update scheduler implementation accordingly.

Thoughts?

Hmmm... not sure why the inheritance is a problem..
But I agree, maybe a better approach would be containment:
Have a SchedulerResourceRequest wrapper class that takes either a standard ResourceRequest or UpdateContainerRequest in the constructor.

Have a SchedulerResourceRequest wrapper class that takes either a standard ResourceRequest or UpdateContainerRequest in the constructor.

If AbstractResourceRequest is only created for:

public void normalizeRequest(AbstractResourceRequest ask)

I would prefer to rename it to: normalizeCapability, change parameter from AbstractResourceRequest to Resource, returns normalized Resource for input Resource.
Or we can direct invoke setMemory/setVirtualCores to update the Resource in-place.

Thoughts?

So, looks like the original class was created to unify Update requests and new Resource Request. So I think we should capture that in the new class.
Given that the Scheduler deals with resources as requests and returns containers as response, I was thinking it should have atleast these 3 fields:

1. Resource capability
2. ContainerId containerToUpdate (defaults to null)
3. int numContainers (if containerToUpdate != null, this will be 1)

I would prefer to think about interface of SchedulerResourceRequest (or AbstractSchedulerResourceRequest) under the context of YARN-5959, I agree to add the class if you plan to use it to solve other issues like unifying UpdatedContainerRequest and ResourceRequest in allocation logic. If it is only for resource normalization, I prefer to add a specific normalize resource method to scheduler.

For example, with this change, we need to make AbstractResourceRequest to be public/stable

Why? Why not leave the AbstractResourceRequest as @Public @Unstable? That way, users will not be able to use AbstractResourceRequest directly but the methods on ResourceRequest remain Public-Stable.

I may be missing the incompatibility part. Wangda Tan - mind clarifying it for me may be with an example of what fails to work now?

Remove 2.8 from target version given YARN-5774 was not actually in branch-2.8.

Wangda Tan, Karthik Kambatla, do you guys want to just move this to @Public @Unstable now (thereby unblocking the release) and maybe refactor this later ?

Karthik Kambatla / Arun Suresh,

Missed your last comment somehow, apologize for the last reply. Since this is not included by branch-2.8, so we should have enough time to change it.

I want to revert the change because two part:
1) It makes a stable API inherit from an unstable API, which is very confusing. Even if we don't want user to use the AbstractRR, but it is unavoidable. According to API definition, user should write code like:

AbstractResourceRequest r = new ResourceRequest(...);
r.setCapability(..._

Apparently we don't want user to do things like this because AbstractRR could be changed.

2) Most importantly, it is not necessary. YARN-5774 only requires scheduler returns normalized resource instead of update resource inside a given resource request. To me, we should avoid changing user-facing API because of internal implementation, this change is completely avoidable. I suggest to keep the original API.

Thoughts?

According to API definition, user should write code like:

AbstractResourceRequest r = new ResourceRequest(...);
r.setCapability(..._

I don't quite follow that. Why wouldn't the user write the same thing as before:

ResourceRequest r = new ResourceRequest(...);
r.setCapability(..._

?

I agree that internal changes should not cause external API changes, but this change doesn't change the API in any meaningful way. No user code will break. Nothing will change about the way users write code. The only change is that the compatibility accessors move from the top section of the Method Summary section in the ResourceRequest and UpdateContainerRequest javadocs into the Methods inherited from... part of the Method Summary section. I did a little digging and was unable to find any wisdom on the interwebs that labels a change like this as a breaking change. (The most complete doc I found was https://wiki.eclipse.org/Evolving_Java-based_APIs_2.)

It's a little bit of an odd API change, but I don't think it's the end of days. Wangda Tan's suggestion on YARN-5774 to change the normalize API to accept a Resource instead of a request sounds like a cleaner solution. I'm fine with implementing that change as long as we can do it before it gets in the way of beta1.

Why wouldn't the user write the same thing as before:

Because it is a public/stable interface, how you stop user to do that?

I don't think it's the end of days ..

It's a little bit of an odd API change

I'm fine with implementing that change as long as we can do it before it gets in the way of beta1.

Fine if you think this solution is fine, and previous one is odd. I can fix it.

Assigning to myself.

Attached ver.1 patch for review.

Just forgot one thing, UpdateContainerRequest should be public/unstable. That is another problem created by YARN-5744, which makes common fields of ResourceRequest/UpdateContainerRequest must have identical visibility. (002)

Looks generally good. Thanks, Wangda Tan. My only comment is that AbstractYarnScheduler.getNormalizeResource() should be AbstractYarnScheduler.getNormalizedResource().

Thanks for review, Daniel Templeton, attached 003 patch addressed your comment.

-1 overall

This message was automatically generated.

The last patch looks clean. Looking at it, though, request.setCapability(scheduler.getNormalizedResource(request.getCapability())) seems like a really clumsy construct. Would it be less awkward to add a @Private normalize method to the request classes so we could call request.normalize(scheduler) instead?

Daniel Templeton, I'm afraid not, since ResourceRequest doesn't have accept to scheduler Java project. Another approach is to use Resource.set(...) method to update fields of Resource in-place, however, when Resource is a read-only instance (like Resources.NONE), it throws runtime exception and bring down RM. So I think we may need to keep it like this.

Fair point. Fine, let's just get this in. +1 I'll commit tomorrow if no one else pops up with any concerns.

Sorry, Wangda Tan, there's a conflict now. Mind rebasing?

Bah, nevermind. The conflict is trivial. I'll take care of it.

Rebased patch. I'll commit after Jenkins gives me the all-clear.

Thanks Daniel Templeton!

-1 overall

This message was automatically generated.

Fixed checkstyle and javadoc issues.

-1 overall

This message was automatically generated.

Now that Jenkins is happy, I'll commit this afternoon.

-1 overall

This message was automatically generated.

There was a trivial conflict in branch-2. Here's the adjusted patch.

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11084 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11084/)
YARN-6022. Revert changes of AbstractResourceRequest (Contributed by (templedf: rev ac16400e1fb85a4186e5bf5bbc9cf204735ae74f)

• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ResourceRequest.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairScheduler.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/UpdateContainerRequest.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMServerUtils.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/YarnScheduler.java
• (delete) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/AbstractResourceRequest.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/SchedulerUtils.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/scheduler/SchedulerRequestKey.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractYarnScheduler.java

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Oops. Screwed up patch5. Hopefully this works better.

-1 overall

This message was automatically generated.

Wangda Tan, since I did the branch-2 patch, do I need your +1 to commit it?

Daniel Templeton,

Really appreciate your help for updating branch-2 patch. +1 and will commit it soon.

Committed to branch-2, and it is already committed to trunk.

Thanks reviews from Daniel Templeton/Arun Suresh/Junping Du/Karthik Kambatla!