Skip to content

Conversation

@prateekm
Copy link
Contributor

@prateekm prateekm commented Mar 7, 2023

Issues: ContainerStorageManager has gotten very unwieldy with 1000+ LOC of highly complex and invocation-order dependent logic. It also does too much by trying to manage side inputs lifecycle along with regular state stores'.

Changes:
1. Separated side inputs (and standby) related store creation and restore logic into a separate SideInputsManager class.
2. Separated helper methods into a ContainerStorageManagerUtil class, and made them all static to ensure that they don't mutate class fields.

Most of the PR is simply moving code from one place to another. However a couple of places that introduce semantic changes or otherwise require careful review are called inline in "Refactor Note" comments.

There is a lot more room for improvement (e.g. more javadocs, more tests, simplifying method signatures, etc). But this change should make it easier to reason about the lifecycle of side inputs for now.

Tests: Existing ContainerStorageManager unit tests pass.

@prateekm
Copy link
Contributor Author

prateekm commented Mar 7, 2023

cc @mynameborat to review.

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 looks good to me except the two comments.

Generally, I noticed you expanded the args list for the utility methods to include instance variables which were implicitly used to be explicit. However, is there any reason not to preserve the ordering of parameters from the existing methods? Would have been much easier to read, compare and review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants