-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update airbyte protocol migration #20745
Changes from 4 commits
e2146b9
caa1692
a374a62
7adbe36
dbad5bb
dce7362
c8275d2
f9717cd
c083b64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
*/ | ||
|
||
package io.airbyte.commons.protocol; | ||
|
||
import io.airbyte.commons.version.Version; | ||
import jakarta.inject.Singleton; | ||
|
||
/** | ||
* Factory to build AirbyteMessageVersionedMigrator | ||
*/ | ||
@Singleton | ||
public class AirbyteProtocolVersionedMigratorFactory { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be an actual micronaut factory (the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the current structure, we inject the factory so that the worker can get a version dependent instance. Because this is per request, I'd expect a bigger refactoring to be needed for this. |
||
|
||
private final AirbyteMessageMigrator airbyteMessageMigrator; | ||
private final ConfiguredAirbyteCatalogMigrator configuredAirbyteCatalogMigrator; | ||
|
||
public AirbyteProtocolVersionedMigratorFactory(final AirbyteMessageMigrator airbyteMessageMigrator, | ||
final ConfiguredAirbyteCatalogMigrator configuredAirbyteCatalogMigrator) { | ||
this.airbyteMessageMigrator = airbyteMessageMigrator; | ||
this.configuredAirbyteCatalogMigrator = configuredAirbyteCatalogMigrator; | ||
} | ||
|
||
public <T> AirbyteMessageVersionedMigrator<T> getAirbyteMessageMigrator(final Version version) { | ||
return new AirbyteMessageVersionedMigrator<>(this.airbyteMessageMigrator, version); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed. |
||
} | ||
|
||
public final VersionedProtocolSerializer getProtocolSerializer(final Version version) { | ||
return new VersionedProtocolSerializer(configuredAirbyteCatalogMigrator, version); | ||
} | ||
|
||
public Version getMostRecentVersion() { | ||
return airbyteMessageMigrator.getMostRecentVersion(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/* | ||
* Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
*/ | ||
|
||
package io.airbyte.commons.protocol; | ||
|
||
import com.google.common.annotations.VisibleForTesting; | ||
import io.airbyte.commons.protocol.migrations.ConfiguredAirbyteCatalogMigration; | ||
import io.airbyte.commons.protocol.migrations.MigrationContainer; | ||
import io.airbyte.commons.version.Version; | ||
import jakarta.annotation.PostConstruct; | ||
import jakarta.inject.Singleton; | ||
import java.util.List; | ||
import java.util.Set; | ||
|
||
@Singleton | ||
public class ConfiguredAirbyteCatalogMigrator { | ||
|
||
private final MigrationContainer<ConfiguredAirbyteCatalogMigration<?, ?>> migrationContainer; | ||
|
||
public ConfiguredAirbyteCatalogMigrator(final List<ConfiguredAirbyteCatalogMigration<?, ?>> migrations) { | ||
migrationContainer = new MigrationContainer<>(migrations); | ||
} | ||
|
||
@PostConstruct | ||
public void initialize() { | ||
migrationContainer.initialize(); | ||
} | ||
|
||
/** | ||
* Downgrade a message from the most recent version to the target version by chaining all the | ||
* required migrations | ||
*/ | ||
public <PreviousVersion, CurrentVersion> PreviousVersion downgrade(final CurrentVersion message, final Version target) { | ||
return migrationContainer.downgrade(message, target, ConfiguredAirbyteCatalogMigrator::applyDowngrade); | ||
} | ||
Comment on lines
+34
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to validate that this version is indeed a downgrade? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so, current design enforces a one-way look up from/to the current version. If the version provided isn't a valid downgrade or upgrade, we either fail because the version isn't supported (in the list of known migrations) or a mismatch in the type conversion. |
||
|
||
/** | ||
* Upgrade a message from the source version to the most recent version by chaining all the required | ||
* migrations | ||
*/ | ||
public <PreviousVersion, CurrentVersion> CurrentVersion upgrade(final PreviousVersion message, final Version source) { | ||
return migrationContainer.upgrade(message, source, ConfiguredAirbyteCatalogMigrator::applyUpgrade); | ||
} | ||
Comment on lines
+42
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to validate that this version is indeed an upgrade? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. answered above. |
||
|
||
public Version getMostRecentVersion() { | ||
return migrationContainer.getMostRecentVersion(); | ||
} | ||
|
||
// Helper function to work around type casting | ||
private static <PreviousVersion, CurrentVersion> PreviousVersion applyDowngrade(final ConfiguredAirbyteCatalogMigration<PreviousVersion, CurrentVersion> migration, | ||
final Object message) { | ||
Comment on lines
+51
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this method is generic, can you have |
||
return migration.downgrade((CurrentVersion) message); | ||
} | ||
|
||
// Helper function to work around type casting | ||
private static <PreviousVersion, CurrentVersion> CurrentVersion applyUpgrade(final ConfiguredAirbyteCatalogMigration<PreviousVersion, CurrentVersion> migration, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this method is generic, can you have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not able to make it work because of the |
||
final Object message) { | ||
return migration.upgrade((PreviousVersion) message); | ||
} | ||
|
||
// Used for inspection of the injection | ||
@VisibleForTesting | ||
Set<String> getMigrationKeys() { | ||
return migrationContainer.getMigrationKeys(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/* | ||
* Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
*/ | ||
|
||
package io.airbyte.commons.protocol; | ||
|
||
import io.airbyte.commons.json.Jsons; | ||
import io.airbyte.protocol.models.ConfiguredAirbyteCatalog; | ||
|
||
public class DefaultProtocolSerializer implements ProtocolSerializer { | ||
|
||
@Override | ||
public String serialize(ConfiguredAirbyteCatalog configuredAirbyteCatalog) { | ||
return Jsons.serialize(configuredAirbyteCatalog); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/* | ||
* Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
*/ | ||
|
||
package io.airbyte.commons.protocol; | ||
|
||
import io.airbyte.protocol.models.ConfiguredAirbyteCatalog; | ||
|
||
public interface ProtocolSerializer { | ||
|
||
String serialize(final ConfiguredAirbyteCatalog configuredAirbyteCatalog); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
*/ | ||
|
||
package io.airbyte.commons.protocol; | ||
|
||
import io.airbyte.commons.json.Jsons; | ||
import io.airbyte.commons.version.Version; | ||
import io.airbyte.protocol.models.ConfiguredAirbyteCatalog; | ||
|
||
public class VersionedProtocolSerializer implements ProtocolSerializer { | ||
|
||
private final ConfiguredAirbyteCatalogMigrator configuredAirbyteCatalogMigrator; | ||
private final Version protocolVersion; | ||
|
||
public VersionedProtocolSerializer(final ConfiguredAirbyteCatalogMigrator configuredAirbyteCatalogMigrator, final Version protocolVersion) { | ||
this.configuredAirbyteCatalogMigrator = configuredAirbyteCatalogMigrator; | ||
this.protocolVersion = protocolVersion; | ||
} | ||
|
||
@Override | ||
public String serialize(final ConfiguredAirbyteCatalog configuredAirbyteCatalog) { | ||
return Jsons.serialize(configuredAirbyteCatalogMigrator.downgrade(configuredAirbyteCatalog, protocolVersion)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this call downgrade? Probably worth throwing a comment into the code here explaining the logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, added some docs to the |
||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the
MigrationContainer
be injected here instead of theList<>
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, the intent is to inject the list of migrations. We have different migrations intended for the
AirbyteMessageMigrator
or theConfiguredAirbyteCatalogMigrator
.The
MigrationContainer<>
is a shared container for abstracting the logic of how we fetch the migrations. Injecting the MigrationContainer would involve an extra layer of subclassing which looks more like overhead than solving a problem at the moment.