You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/09/30 07:17:32 UTC
[GitHub] [spark] khalidmammadov opened a new pull request, #38058: [SPARK-40620] [CORE] Simplify make offers
khalidmammadov opened a new pull request, #38058:
URL: https://github.com/apache/spark/pull/38058
### What changes were proposed in this pull request?
WorkerOffer build in CoarseGrainedSchedulerBackend is repeated two different places with exact same parameters. We can simplify, deduplicate and increase readability by moving that to a private function
### Why are the changes needed?
Code deduplication and increase readability.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Local build and test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on pull request #38058: [SPARK-40620] [CORE] Simplify make offers
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38058:
URL: https://github.com/apache/spark/pull/38058#issuecomment-1263910346
If we ever wish to tackle this, will that make more sense to adopt `Builder` pattern that have every place of `new WorkerOffer` to go through such `buildWorkerOffer`?
I found 80+ place that use `new WorkerOffer` and they can be converted to `WorkerOffer.Builder().addExecutorId().addExecutorData().addHost()....`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] khalidmammadov commented on pull request #38058: [SPARK-40620] [CORE] Simplify make offers
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on PR #38058:
URL: https://github.com/apache/spark/pull/38058#issuecomment-1264338523
> If we ever wish to tackle this, will that make more sense to adopt `Builder` pattern that have every place of `new WorkerOffer` to go through such `buildWorkerOffer`?
>
> I found 80+ place that use `new WorkerOffer` and they can be converted to `WorkerOffer.Builder().addExecutorId().addExecutorData().addHost()....`.
>
> Seems to me that `buildWorkerOffer` in this PR is more or less trying to go to Builder pattern.
These 80+ are Unit tests aren't they? In reality `WorkerOffer` only used handful of times as part of a `SchedulerBackend` and is not part of user facing API, so not sure how `Builder` pattern will help. This PR is just to remove/replace duplicated code with more readable one as many developers read these codes and when one see this types of dups we start wondering what is the diff.? and only to find out there is none! :) But already energy and time is spent to find this! So, IMHO this PR makes the code more clean, cut and readable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen closed pull request #38058: [SPARK-40620] [CORE] Simplify make offers
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #38058: [SPARK-40620] [CORE] Simplify make offers
URL: https://github.com/apache/spark/pull/38058
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #38058: [SPARK-40620] [CORE] Simplify make offers
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #38058:
URL: https://github.com/apache/spark/pull/38058#issuecomment-1264507359
Merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #38058: [SPARK-40620] [CORE] Simplify make offers
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38058:
URL: https://github.com/apache/spark/pull/38058#issuecomment-1264319379
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] khalidmammadov commented on pull request #38058: [SPARK-40620] [CORE] Simplify make offers
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on PR #38058:
URL: https://github.com/apache/spark/pull/38058#issuecomment-1264445529
@srowen please review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on pull request #38058: [SPARK-40620] [CORE] Simplify make offers
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38058:
URL: https://github.com/apache/spark/pull/38058#issuecomment-1264432610
> > If we ever wish to tackle this, will that make more sense to adopt `Builder` pattern that have every place of `new WorkerOffer` to go through such `buildWorkerOffer`?
> > I found 80+ place that use `new WorkerOffer` and they can be converted to `WorkerOffer.Builder().addExecutorId().addExecutorData().addHost()....`.
> > Seems to me that `buildWorkerOffer` in this PR is more or less trying to go to Builder pattern.
>
> These 80+ are Unit tests aren't they? In reality `WorkerOffer` only used handful of times as part of a `SchedulerBackend` and is not part of user facing API, so not sure how `Builder` pattern will help. This PR is just to remove/replace duplicated code with more readable one as many developers read these codes and when one see this types of dups we start wondering what is the diff.? and only to find out there is none! :) But already energy and time is spent to find this! So, IMHO this PR makes the code more clean, cut and readable.
I agree that this PR is useful. I was just thinking that if we can make it help other places. Let's see if other people in community has an opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org