Extract StandardSyncPersistence from DatabaseConfigPersistence #18930
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gosusnp
requested review from
colesnodgrass,
jdpgrailsdev,
benmoriceau and
alovew
jdpgrailsdev
reviewed
Nov 3, 2022
| } | ||
|
|
||
| @VisibleForTesting | ||
| ConfigRepository(final ConfigPersistence persistence, final Database database, final ActorDefinitionMigrator actorDefinitionMigrator) { | ||
| ConfigRepository(final ConfigPersistence persistence, |
There was a problem hiding this comment.
It may make sense to just make this constructor the one, public constructor in order to better support dependency injection and better support inversion of control.
There was a problem hiding this comment.
I agree that's how we should be doing it eventually, for now, I'd rather not to avoid extra breaking changes with cloud.
jdpgrailsdev
approved these changes
Nov 3, 2022
LGTM -- just one nit about the ConfigRepository constructor, but that change can be done as a separate PR, as it will also introduce some breaking changes.
benmoriceau
approved these changes
Nov 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
ConfigRepositoryis getting too big, breaking it out into more specific classes would help separation of concerns and more specific dependency injection.ConfigPersistenceand its implementation are convoluted with some form of fan in/out dispatch on its operation. We want to move away from those objects.How
Extract
StandardSyncrelated operations into aStandardSyncPersistenceas a first step.It is still used under the hood by
ConfigRepository, to minimize the span of this change.Medium term next step would be to have clients use the
StandardSyncPersistencedirectly, but this step should become more straightforward once the APIs are broken up and we are using micronaut in theairbyte-server.Recommended reading order
StandardSyncPersistencecontains the code that has been extracted fromDatabaseConfigPersistence. Logic change should be minimal, mostly moving code around.