-
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
Add some abstraction to avoid direct generated object use #20905
Conversation
Affected Connector ReportNOTE
|
Connector | Version | Changelog | Publish |
---|---|---|---|
source-alloydb |
1.0.34 |
✅ | ✅ |
source-alloydb-strict-encrypt |
1.0.34 |
🔵 (ignored) |
🔵 (ignored) |
source-bigquery |
0.2.3 |
✅ | ✅ |
source-clickhouse |
0.1.14 |
✅ | ✅ |
source-clickhouse-strict-encrypt |
0.1.14 |
🔵 (ignored) |
🔵 (ignored) |
source-cockroachdb |
0.1.18 |
✅ | ✅ |
source-cockroachdb-strict-encrypt |
0.1.18 |
🔵 (ignored) |
🔵 (ignored) |
source-db2 |
0.1.16 |
✅ | ✅ |
source-db2-strict-encrypt |
0.1.16 |
🔵 (ignored) |
🔵 (ignored) |
source-dynamodb |
0.1.0 |
✅ | ✅ |
source-e2e-test |
2.1.3 |
✅ | ✅ |
source-e2e-test-cloud |
2.1.1 |
🔵 (ignored) |
🔵 (ignored) |
source-elasticsearch |
0.1.1 |
✅ | ✅ |
source-jdbc |
0.3.5 |
🔵 (ignored) |
🔵 (ignored) |
source-kafka |
0.2.3 |
✅ | ✅ |
source-mongodb-strict-encrypt |
0.1.19 |
🔵 (ignored) |
🔵 (ignored) |
source-mongodb-v2 |
0.1.19 |
✅ | ✅ |
source-mssql |
0.4.26 |
✅ | ✅ |
source-mssql-strict-encrypt |
0.4.26 |
🔵 (ignored) |
🔵 (ignored) |
source-mysql |
1.0.18 |
✅ | ✅ |
source-mysql-strict-encrypt |
1.0.18 |
🔵 (ignored) |
🔵 (ignored) |
source-oracle |
0.3.21 |
✅ | ✅ |
source-oracle-strict-encrypt |
0.3.21 |
🔵 (ignored) |
🔵 (ignored) |
source-postgres |
1.0.34 |
✅ | ✅ |
source-postgres-strict-encrypt |
1.0.34 |
🔵 (ignored) |
🔵 (ignored) |
source-redshift |
0.3.15 |
✅ | ✅ |
source-scaffold-java-jdbc |
0.1.0 |
🔵 (ignored) |
🔵 (ignored) |
source-sftp |
0.1.2 |
✅ | ✅ |
source-snowflake |
0.1.27 |
✅ | ✅ |
source-tidb |
0.2.1 |
✅ | ✅ |
- See "Actionable Items" below for how to resolve warnings and errors.
❌ Destinations (47)
Connector | Version | Changelog | Publish |
---|---|---|---|
destination-aws-datalake |
0.1.1 |
✅ | ✅ |
destination-azure-blob-storage |
0.1.6 |
✅ | ✅ |
destination-bigquery |
1.2.9 |
✅ | ✅ |
destination-bigquery-denormalized |
1.2.9 |
✅ | ✅ |
destination-cassandra |
0.1.4 |
✅ | ✅ |
destination-clickhouse |
0.2.1 |
✅ | ✅ |
destination-clickhouse-strict-encrypt |
0.2.1 |
🔵 (ignored) |
🔵 (ignored) |
destination-csv |
0.2.10 |
✅ | ✅ |
destination-databricks |
0.3.1 |
✅ | ✅ |
destination-dev-null |
0.2.7 |
🔵 (ignored) |
🔵 (ignored) |
destination-doris |
0.1.0 |
✅ | ✅ |
destination-dynamodb |
0.1.7 |
✅ | ✅ |
destination-e2e-test |
0.2.4 |
✅ | ✅ |
destination-elasticsearch |
0.1.6 |
✅ | ✅ |
destination-elasticsearch-strict-encrypt |
0.1.6 |
🔵 (ignored) |
🔵 (ignored) |
destination-gcs |
0.2.12 |
✅ | ✅ |
destination-iceberg |
0.1.0 |
✅ | ✅ |
destination-jdbc |
0.3.14 |
🔵 (ignored) |
🔵 (ignored) |
destination-kafka |
0.1.10 |
✅ | ✅ |
destination-keen |
0.2.4 |
✅ | ✅ |
destination-kinesis |
0.1.5 |
✅ | ✅ |
destination-local-json |
0.2.11 |
✅ | ✅ |
destination-mariadb-columnstore |
0.1.7 |
✅ | ✅ |
destination-mongodb |
0.1.9 |
✅ | ✅ |
destination-mongodb-strict-encrypt |
0.1.9 |
🔵 (ignored) |
🔵 (ignored) |
destination-mqtt |
0.1.3 |
✅ | ✅ |
destination-mssql |
0.1.22 |
✅ | ✅ |
destination-mssql-strict-encrypt |
0.1.22 |
🔵 (ignored) |
🔵 (ignored) |
destination-mysql |
0.1.20 |
✅ | ✅ |
destination-mysql-strict-encrypt |
❌ 0.1.21 (mismatch: 0.1.20 ) |
🔵 (ignored) |
🔵 (ignored) |
destination-oracle |
0.1.19 |
✅ | ✅ |
destination-oracle-strict-encrypt |
0.1.19 |
🔵 (ignored) |
🔵 (ignored) |
destination-postgres |
0.3.26 |
✅ | ✅ |
destination-postgres-strict-encrypt |
0.3.26 |
🔵 (ignored) |
🔵 (ignored) |
destination-pubsub |
0.2.0 |
✅ | ✅ |
destination-pulsar |
0.1.3 |
✅ | ✅ |
destination-r2 |
0.1.0 |
✅ | ✅ |
destination-redis |
0.1.4 |
✅ | ✅ |
destination-redpanda |
0.1.0 |
✅ | ✅ |
destination-redshift |
0.3.51 |
✅ | ✅ |
destination-rockset |
0.1.4 |
✅ | ✅ |
destination-s3 |
0.3.18 |
✅ | ✅ |
destination-s3-glue |
0.1.1 |
✅ | ✅ |
destination-scylla |
0.1.3 |
✅ | ✅ |
destination-snowflake |
0.4.40 |
✅ | ✅ |
destination-tidb |
0.1.0 |
✅ | ✅ |
destination-yugabytedb |
0.1.0 |
✅ | ✅ |
- See "Actionable Items" below for how to resolve warnings and errors.
✅ Other Modules (0)
Actionable Items
(click to expand)
Category | Status | Actionable Item |
---|---|---|
Version | ❌ mismatch |
The version of the connector is different from its normal variant. Please bump the version of the connector. |
⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
|
Changelog | ⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
❌ changelog missing |
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog. | |
Publish | ⚠ not in seed |
The connector is not in the seed file (e.g. source_definitions.yaml ), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug. |
❌ diff seed version |
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version. |
/test connector=connectors/source-postgres
Build PassedTest summary info:
|
/test connector=connectors/destination-snowflake |
/test connector=connectors/destination-snowflake
Build PassedTest summary info:
|
@edgao, @akashkulk, in our previous discussions, I mentioned we should look into using interfaces rather than generated objects directly. This PR is trying to illustrate what this could look like. |
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.
took a super quick first pass on this - added a few questions before I take a deeper look
import lombok.EqualsAndHashCode; | ||
|
||
@EqualsAndHashCode | ||
public class ConnectorSpecificationAdapter implements ConnectorSpecification { |
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.
checking my understanding: is the idea that we would have an adapter for every protocol version?
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.
Not necessarily, the interface gives us this flexibility, the idea is that the adapter should be the only place where we need bump the objects.
import java.net.URI; | ||
import java.util.List; | ||
|
||
@JsonDeserialize(using = ConnectionSpecificationDeserializer.class) |
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 these be declared on the adapters? E.g. if the platform is on protocol v42, but an older connector still uses protocol v41, and it has a call to Jsons.serialize(new ConnectorSpecificationAdapterV41(...))
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.
Those are needed for jackson to know how to serialize the objects of the model. In this case, it goes on the interface because I declared the interface in the ConnectorJobOutput.yaml
rather than the implementation.
import lombok.EqualsAndHashCode; | ||
|
||
@EqualsAndHashCode | ||
public class ConnectorSpecificationAdapter implements ConnectorSpecification { |
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.
also, how sketchy is it to have the generated protocol models directly implement the interfaces? I.e.
- interface declares
default Whatever getWhatever() { return null; }
- and the generated classes look like
public class AirbyteMessage implements io.airbyte.commons.protocol.objects.AirbyteMessage
then:
- adding new fields is still easy (just add a new
default
method to the interface) - deleting fields is still easy (mark the interface method as deprecated, or just delete it entirely)
- renaming fields is... probably fine? (
default Whatever getWhatever() { return getOldWhatever(); }
)
(I forget if the issue was that there's a technical problem, or that the code generator just doesn't support this - if the latter, we could maaaaybe do something similar to https://github.com/LiveRamp/extravagance/ and edit the generated code)
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.
One objective is to decouple the generated code from the how we use it.
For example, we may decide to rename field in the protocol, by decoupling this, we can control if/when we want to propagate this. Typical reason could be to limit the amount of changes at the same time.
When it comes to deprecation, newer version could remove some fields, those fields could remain in the interface, but we would have the option to throw exceptions depending on the adapter implementation.
@Override | ||
public void serialize(ConnectorSpecification value, JsonGenerator gen, SerializerProvider provider) throws IOException { | ||
gen.writeStartObject(); | ||
gen.writeStringField("object", value.toJson()); |
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.
which components would receive these {object: {spec...}}
messages? I.e. do connectors need to produce this format? Or are connectors still expected to produce protocol messages, and then the worker would translate them into this format? (or something else?)
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 think those should remain within the platform, those objects should be mostly used for temporal payloads which is generated by the clients we use and the activity. It currently leaks to the connector's tests because some tests rely on the workers.
Connectors having to generate those should be a red flag.
Jimmy, going to close this for now. Feel free to move this on to airbyte-platform if it's relevant! |
What
With the introduction of Airbyte Protocol versioning, direct use of the generated objects comes with some overhead:
import
statement of classes that are relying on protocol objectsHow
In order to reduce the coupling:
The key benefits:
The tradeoff is that we need to explicitly update the interfaces/adapters to channel new information.
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
No behavior change expected, this is targeting dev efficiency.