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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
