Skip to content

SAMZA-2605: Make Standby Container Requests Rack Aware #1446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mynameborat merged 15 commits into from
Dec 24, 2020
Merged

SAMZA-2605: Make Standby Container Requests Rack Aware #1446

mynameborat merged 15 commits into from
Dec 24, 2020

Conversation

@PawasChhokra
Copy link
Contributor

@PawasChhokra PawasChhokra commented Nov 20, 2020 •
edited
Loading

Feature: The aim of this feature is to make all standby container requests rack aware such that all active containers and their corresponding standby containers are always on different racks. This helps with decreased downtime of applications during rack failures.

One of the requirements of this feature is that the value of job.standbytasks.replication.factor is at max 2 for the rack awareness functionality to be honored.

Changes: This PR uses the FaultDomainManager interface for Yarn to request for rack aware nodes while making standby container requests.

API Changes: None

Tests: Added

Upgrade instructions: TBD

Usage Instructions: For a job with host affinity and standby containers, set the config cluster-manager.fault-domain-aware.standby.enabled to true to enable this feature.

lakshmi-manasa-g
lakshmi-manasa-g reviewed Dec 4, 2020
View reviewed changes
Copy link
Contributor

@lakshmi-manasa-g lakshmi-manasa-g left a comment

Choose a reason for hiding this comment

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

first pass half way through. will come back to the other half in a bit.

lakshmi-manasa-g
lakshmi-manasa-g reviewed Dec 7, 2020
View reviewed changes
Copy link
Contributor

@lakshmi-manasa-g lakshmi-manasa-g left a comment

Choose a reason for hiding this comment

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

finished one pass.

@mynameborat mynameborat changed the title [WIP] SAMZA-2605: Make Standby Container Requests Rack Aware SAMZA-2605: Make Standby Container Requests Rack Aware Dec 7, 2020
mynameborat
mynameborat reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@mynameborat mynameborat left a comment

Choose a reason for hiding this comment

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

Mostly agree to Manasa's comments. Added few major ones around FaultDomainManager interface and RackManager.

It is definitely looking much better than the previous version :)

samza-core/src/main/java/org/apache/samza/clustermanager/FaultDomainManager.java Outdated
* @param host the host
* @return the {@link FaultDomain}
*/
FaultDomain getFaultDomainOfNode(String host);
Copy link
Contributor

@mynameborat mynameborat Dec 8, 2020

Choose a reason for hiding this comment

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

+1 use consistent terminologies.

lakshmi-manasa-g
lakshmi-manasa-g reviewed Dec 10, 2020
View reviewed changes
Copy link
Contributor

@lakshmi-manasa-g lakshmi-manasa-g left a comment

Choose a reason for hiding this comment

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

thank you for addressing all the feedback.. just one clarification.

mynameborat
mynameborat reviewed Dec 15, 2020
View reviewed changes
Copy link
Contributor

@mynameborat mynameborat left a comment

Choose a reason for hiding this comment

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

Took another pass. Any reason why the new parameters do in the middle of the signature instead of end?

mynameborat
mynameborat reviewed Dec 15, 2020
View reviewed changes
Copy link
Contributor

@mynameborat mynameborat left a comment

Choose a reason for hiding this comment

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

Few comments around the configuration. Once you resolve them, can we update the configuration table docs to reflect details about these?

Specifically, which set of configurations are to be turned on in tandem for the feature to work and what are the defaults provided.

mynameborat
mynameborat reviewed Dec 17, 2020
View reviewed changes
Copy link
Contributor

@mynameborat mynameborat left a comment

Choose a reason for hiding this comment

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

Thanks for pushing through this. Few follow ups.
Looks good otherwise.

lakshmi-manasa-g
lakshmi-manasa-g reviewed Dec 17, 2020
View reviewed changes
Copy link
Contributor

@lakshmi-manasa-g lakshmi-manasa-g left a comment

Choose a reason for hiding this comment

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

a few minor comments.

mynameborat
mynameborat reviewed Dec 21, 2020
View reviewed changes
@mynameborat
Copy link
Contributor

mynameborat commented Dec 21, 2020

A few consistency related comments. Please resolve the conflicts as well.

@PawasChhokra PawasChhokra force-pushed the RackAwareContainerRequest branch from c5e52cf to b871543 Compare December 23, 2020 03:26
mynameborat
mynameborat reviewed Dec 23, 2020
View reviewed changes
Copy link
Contributor

@mynameborat mynameborat left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Can you update where you landed on adding the invariant of non-empty activeContainerId within issueStandByAwareAllocation method?

@PawasChhokra PawasChhokra force-pushed the RackAwareContainerRequest branch from c0960b3 to 8ed4575 Compare December 24, 2020 03:59
@mynameborat mynameborat merged commit 5748fa6 into apache:master Dec 24, 2020
lakshmi-manasa-g pushed a commit to lakshmi-manasa-g/samza that referenced this pull request Feb 9, 2021
@PawasChhokra @lakshmi-manasa-g
SAMZA-2605: Make Standby Container Requests Rack Aware (apache#1446)
be92b2c 
Feature: The aim of this feature is to make all standby container requests rack aware such that all active containers and their corresponding standby containers are always on different racks. This helps with decreased downtime of applications during rack failures.

One of the requirements of this feature is that the value of job.standbytasks.replication.factor is at max 2 for the rack awareness functionality to be honored.

Changes: This PR uses the FaultDomainManager interface for Yarn to request for rack aware nodes while making standby container requests.

Usage Instructions: For a job with host affinity and standby containers, set the config cluster-manager.fault-domain-aware.standby.enabled to true to enable this feature.
tranjith pushed a commit to tranjith/samza that referenced this pull request Mar 23, 2021
@PawasChhokra @tranjith
SAMZA-2605: Make Standby Container Requests Rack Aware (apache#1446)
662fb98 
Feature: The aim of this feature is to make all standby container requests rack aware such that all active containers and their corresponding standby containers are always on different racks. This helps with decreased downtime of applications during rack failures.

One of the requirements of this feature is that the value of job.standbytasks.replication.factor is at max 2 for the rack awareness functionality to be honored.

Changes: This PR uses the FaultDomainManager interface for Yarn to request for rack aware nodes while making standby container requests.

Usage Instructions: For a job with host affinity and standby containers, set the config cluster-manager.fault-domain-aware.standby.enabled to true to enable this feature.
shekhars-li pushed a commit to shekhars-li/samza that referenced this pull request May 28, 2021
@PawasChhokra @mynameborat
SAMZA-2605: Make Standby Container Requests Rack Aware (apache#1446)
e9e49af 
Feature: The aim of this feature is to make all standby container requests rack aware such that all active containers and their corresponding standby containers are always on different racks. This helps with decreased downtime of applications during rack failures.

One of the requirements of this feature is that the value of job.standbytasks.replication.factor is at max 2 for the rack awareness functionality to be honored.

Changes: This PR uses the FaultDomainManager interface for Yarn to request for rack aware nodes while making standby container requests.

Usage Instructions: For a job with host affinity and standby containers, set the config cluster-manager.fault-domain-aware.standby.enabled to true to enable this feature.
shekhars-li pushed a commit to shekhars-li/samza that referenced this pull request May 28, 2021
@lakshmi-manasa-g
Revert "SAMZA-2605: Make Standby Container Requests Rack Aware (apach…
bb362bf 
…e#1446)"

This reverts commit e9e49af.

RB=2485434
BUG=LISAMZA-20154
G=samza-reviewers
R=calee,rmatharu
A=rmatharu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mynameborat mynameborat mynameborat left review comments

+1 more reviewer

@lakshmi-manasa-g lakshmi-manasa-g lakshmi-manasa-g left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PawasChhokra @mynameborat @lakshmi-manasa-g