You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by "mynameborat (via GitHub)" <gi...@apache.org> on 2023/08/16 05:39:46 UTC
[GitHub] [samza] mynameborat opened a new pull request, #1680: SAMZA-2790: Cleanup RunLoop constructor explosion
mynameborat opened a new pull request, #1680:
URL: https://github.com/apache/samza/pull/1680
**Description**:
Runloop currently takes in lot of parameters and the constructor has grown to the point where it is unmanageable with multiple overloads. Introducing new configuration requires lot of updates to existing tests and components even if the parameters have no effect on all of the usages.
With this PR, we should be able to decouple different users of RunLoop and enable these components to have their own scoped config. e.g., `SideInputManager` can now have its own set of runloop parameters without having to tie itself with `TaskConfig`.
**Changes**:
- Introduce `RunLoopConfig`, a container object to hold all required parameters for runloop from `Config`.
- Remove existing overloads of constructor
- Simplify the constructor to take `RunLoopConfig` and initialize the necessary components and fields
- Introduce `SideInputManagerRunLoopConfig`, an overload of `RunLoopConfig` to be used within `SideInputManager`
- Modify RunLoopFactory create method signature
**Tests**:
- Unit test
**API Changes**:
- No external API change
**Usage Instructions**: N.A
**Upgrade Instructions**: N.A
**Note**: This PR serves as foundation for SAMZA-2791
--
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-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 commented on pull request #1680: SAMZA-2790: Cleanup RunLoop constructor explosion
Posted by "Sanil15 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ajothomas commented on pull request #1680: SAMZA-2790: Cleanup RunLoop constructor explosion
Posted by "ajothomas (via GitHub)" <gi...@apache.org>.
ajothomas commented on PR #1680:
URL: https://github.com/apache/samza/pull/1680#issuecomment-1681057307
> > 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.
--
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-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on pull request #1680: SAMZA-2790: Cleanup RunLoop constructor explosion
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on PR #1680:
URL: https://github.com/apache/samza/pull/1680#issuecomment-1681037076
> 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.
--
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-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] Sanil15 commented on a diff in pull request #1680: SAMZA-2790: Cleanup RunLoop constructor explosion
Posted by "Sanil15 (via GitHub)" <gi...@apache.org>.
Sanil15 commented on code in PR #1680:
URL: https://github.com/apache/samza/pull/1680#discussion_r1296219539
##########
samza-core/src/main/scala/org/apache/samza/storage/SideInputsManager.java:
##########
@@ -507,4 +496,32 @@ private static boolean awaitSideInputTasks(Map<TaskName, CountDownLatch> sideInp
}
return true;
}
+
+ private static class SideInputRunLoopConfig extends RunLoopConfig {
+
+ public SideInputRunLoopConfig(Config config) {
+ super(config);
+ }
+
+ @Override
+ public int getMaxConcurrency() {
Review Comment:
minor: Lets add comment here for someone in future to have the context:
Sideinput only processes single message per task and add the no windowing below
--
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-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat merged pull request #1680: SAMZA-2790: Cleanup RunLoop constructor explosion
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat merged PR #1680:
URL: https://github.com/apache/samza/pull/1680
--
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-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on pull request #1680: SAMZA-2790: Cleanup RunLoop constructor explosion
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on PR #1680:
URL: https://github.com/apache/samza/pull/1680#issuecomment-1696166917
@Sanil15 @ajothomas I had missed the changes that made `RunLoopConfig` extends `MapConfig`. Originally, I started with container/holder pattern and then realized I can extend `MapConfig`. Applied my stashed changes.
Feel free to take another look.
--
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-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org