Skip to content
This repository has been archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-1900] Add option for number of cores for the Driver in REEF Java #1388

Closed
wants to merge 2 commits into from

Conversation

markusweimer
Copy link
Contributor

This adds all the plumbing to allow for the configuration of the Driver's CPU cores in the REEF Java implementation.

Notes:

  • Only the YARN and HDInsight runtimes read the new field.

JIRA:
REEF-1900

This adds all the plumbing to allow for the configuration of the Driver's CPU
cores in the REEF Java implementation.

Notes:
  * Only the YARN and HDInsight runtimes read the new field.

JIRA:
  [REEF-1900](https://issues.apache.org/jira/browse/REEF-1900)

Pull Request:
  This closes #
@markusweimer
Copy link
Contributor Author

Tests pass locally, but I have not tested this on YARN.

Copy link
Contributor

@motus motus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @dougmsft will also review and merge it

@dougmsft
Copy link
Contributor

LGTM Sergiy and I will test it on Yarn. I am requesting access to the CISL cluster. I will commit if it passes.

@dougmsft
Copy link
Contributor

Local tests pass. Waiting for access to CISL cluster for YARN testing.

* Number of cores assigned to the Driver Container.
*/
@NamedParameter(doc = "Number of CPU Cores allocated to the Driver.", default_value = "1")
public final class DriverCPUCores implements Name<Integer> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VirtualCores or CPUCores which one is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with the generic CPUCores, as this API is for more than YARN.

@@ -136,12 +139,11 @@ public YarnSubmissionHelper setApplicationName(final String applicationName) {
}

/**
* Set the amount of memory to be allocated to the Driver.
* @param megabytes
* Set the resources for the Driver.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set memory and number of cores to be allocated to the driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I addressed this.

@jwang98052
Copy link
Contributor

I have done a pass. It looks good. Should have a test to set the core or hello to show example and test the code pass.

@markusweimer
Copy link
Contributor Author

Should have a test to set the core or hello to show example and test the code pass.

How would we test this? If we run on YARN, we'd have to restrict the tests to clusters with large enough machines, right?

@markusweimer
Copy link
Contributor Author

@jwang98052 I addressed your comments, Please have another look.

@jwang98052
Copy link
Contributor

Can we set it to event 2 in Hello and run it as a test?

@markusweimer
Copy link
Contributor Author

Can we set it to event 2 in Hello and run it as a test?

We can. But what do we do if the cluster doesn't have 2 core containers? Also, I don't know how to inquire how many cores we actually got to make sure the test passes. Any idea how to do that?

@jwang98052
Copy link
Contributor

I will add it into yarn test once .Net change is made. Other than it, LGTM. I will test and merge.

@asfgit asfgit closed this in 0a48ea9 Oct 20, 2017
@markusweimer markusweimer deleted the REEF-1900 branch October 24, 2017 17:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants