Skip to content
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

Interface changes revision-2 #58

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

manjutapali
Copy link
Collaborator

@manjutapali manjutapali commented Jul 2, 2024

Description

Updated the proto interfaces with

  • Added default value, place holder for FormField.
  • Added new DataType - NAIVE_TIME
  • Renamed OpType to RecordType
  • Removed LogEntry, modified the UpdateResponse method.

@manjutapali manjutapali requested a review from ediril July 2, 2024 17:23
connector_sdk.proto Show resolved Hide resolved
@ediril ediril requested a review from georgewfraser July 5, 2024 05:24
@@ -1,8 +1,8 @@
syntax = "proto3";
option optimize_for = SPEED;
option java_multiple_files = true;
option go_package = "fivetran.com/fivetran_sdk";
package fivetran_sdk;
option go_package = "fivetran.com/fivetran_sdk_r2";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we changing the package name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are introducing breaking changes and will use the identifier revision (r2) to denote the version for these updates.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@georgewfraser as we discussed during the previous meeting, we need to be able to run one version in production while we need to be able to build and release different interface version in the testers to give to the partners. Since the testers are in the engineering repo too, there is no way to have two versions of grpc interface in the same package. So we introduced the "_r2" for "revision 2". (We chose to replace "version" with "revision" to indicate we are not going to use versions)

@@ -55,7 +55,26 @@ message CreateTableResponse {
message AlterTableRequest {
map<string, string> configuration = 1;
string schema_name = 2;
Table table = 3;
string table_name = 3;
repeated SchemaDiff changes = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do our own DataWriter implementations get something like this? How is it generated?

Copy link
Collaborator Author

@manjutapali manjutapali Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, our SDKDataWriter generates the schema difference. The same approach is followed across all the DataWriters.
For eg: In SQLLikeDataWriter, we compute the difference in primary key, update in column type, added new columns etc - SQLLikeDataWriter#updateSchema, SQLLikeDataWriter#updateTable

Copy link
Collaborator

@ediril ediril Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@georgewfraser no they don't "get" it from the core, they calculate it themselves. This is something new we introduced so the partner code doesn't have to calculate it. SdkDataWriter generates the diffs to pass to the partner code so they don't have to do it themselves.

It would be great if we can move this to the core so the other data writers didn't have to perform the diff either. (In fact, the core doesn't even do a simple compare between existing and new schema, calling DataWriter#updateSchema whether the two are different or not, expecting every data writer to figure it out)

common.proto Outdated Show resolved Hide resolved
@georgewfraser
Copy link
Contributor

One more q, how are we doing logging now?

@ediril
Copy link
Collaborator

ediril commented Jul 9, 2024

One more q, how are we doing logging now?

@georgewfraser Logging is handled the same way it is handled for donkey processes, that is, the partner code prints to STDOUT in a specific JSON format. FluentBit collects these, processes them and sends them to New Relic.

@manjutapali
Copy link
Collaborator Author

One more q, how are we doing logging now?

Currently, the logs are sent as a separate LogEntry operation. Going forward Partner's can use STDOUT with specified JSON format.

Note that, as of now the connections logs are not exposed to the Partners.

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.

3 participants