Sanil15 commented on PR #1680: URL: https://github.com/apache/samza/pull/1680#issuecomment-1681061794
> > > I am in agreement with the idea behind this PR i.e to remove all config related params from RunLoop constructor. > > > But, do we need a holder class `RunLoopConfig` though? Please correct me if i am wrong but whatever we are trying to do thru this class can be achieved by passing `Config` object as param to RunLoop, initializing JobConfig, TaskConfig etc in the constructor and setting the member variables directly in the RunLoop constructor. > > > Instead of mocking `RunLoopConfig` in tests, we could have a mock config class with mock configs set. > > > > > > @ajothomas Good question. That is definitely one of approaches I evaluated. Some of my thoughts on that > > > > 1. We end up making constructor a bit heavy and create objects that are short lived just to initialize few fields. e.g., new TaskConfig(...), new JobConfig(...) and so-on which sort of violates DI principles where you'd want dependencies to be passed in as much as possible. > > 2. Authoring tests and mocking components that are not plugged in through constructor is hard and not manageable. > > 3. Goes back to scoping configurations and having multiple components leverage `RunLoop`. The current implementation allows that while passing in a `Config` and creating specific ones within constructor would make extensions more hard as you'd have to modify the Config objects passed by different components to account for code evolution within constructor. > > 4. Lastly, this paves way to evolve configs that are used within `RunLoop` to its own scope instead of having to define it in existing scopes that we have (Task, Application, Job etc) even if it doesn't make sense to logically put them there. e.g., max throttling delay configuration scoped to container. > > > > Let me know your thoughts. > > Fair enough. Just another comment- Do we want to call it something else- `RunLoopConfigHolder` and not `RunLoopConfig`. Traditionally, anything suffixed with `Config` has been extending `MapConfig`/`Config` class so this naming doesn't quite follow the pattern. @mynameborat dont we have already scoped configs for say BlobStore, table ... and they all are called config. All of them extend MapConfig, so whats the concern doing here?  -- 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: commits-unsubscr...@samza.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
