-
Notifications
You must be signed in to change notification settings - Fork 97
feat: Add Kafka Connect Cloud Bigtable sink connector #2466
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Warning: This pull request is touching the following templated files:
|
e2f0361
to
40ae074
Compare
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.
This is shaping up nicely! All my comments are pretty minor. I did not review the Integration tests yet. I'll get back to you soon on how to handle logical types.
.../src/main/java/com/google/cloud/kafka/connect/bigtable/autocreate/BigtableSchemaManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/google/cloud/kafka/connect/bigtable/autocreate/BigtableSchemaManager.java
Outdated
Show resolved
Hide resolved
...-connect-sink/src/main/java/com/google/cloud/kafka/connect/bigtable/mapping/ValueMapper.java
Outdated
Show resolved
Hide resolved
...-connect-sink/src/main/java/com/google/cloud/kafka/connect/bigtable/mapping/ValueMapper.java
Outdated
Show resolved
Hide resolved
...-connect-sink/src/main/java/com/google/cloud/kafka/connect/bigtable/mapping/ValueMapper.java
Outdated
Show resolved
Hide resolved
...onnect-sink/src/test/java/com/google/cloud/kafka/connect/bigtable/mapping/KeyMapperTest.java
Outdated
Show resolved
Hide resolved
...onnect-sink/src/test/java/com/google/cloud/kafka/connect/bigtable/mapping/KeyMapperTest.java
Outdated
Show resolved
Hide resolved
...onnect-sink/src/test/java/com/google/cloud/kafka/connect/bigtable/mapping/KeyMapperTest.java
Outdated
Show resolved
Hide resolved
...onnect-sink/src/test/java/com/google/cloud/kafka/connect/bigtable/mapping/KeyMapperTest.java
Outdated
Show resolved
Hide resolved
...onnect-sink/src/test/java/com/google/cloud/kafka/connect/bigtable/mapping/KeyMapperTest.java
Outdated
Show resolved
Hide resolved
@brandtnewton I also have a question: how do you want to review further commits (mainly logical types support, some more integration tests, and some minor tweaks throughout the codebase)? In this PR? In a new one? Or maybe do you want to create a new repository and have the PR(s) sent there? Please let me know. |
@prawilny you can just add those commits to this PR. GitHub makes it easy to see only the changes since my last review |
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.
Thanks for all the changes! Feel free to add more changes here or in another PR, whatever is easier for you. See comments:
@brandtnewton, please take another look. |
… casing ConfluentCompatibilityIT
…families before the integration tests
…fications before starting to read table rows
…urce auto creation
@brandtnewton, please take a look. |
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.
There might be some junk files in here but otherwise lgtm
@@ -0,0 +1,69 @@ | |||
diff --git a/google-cloud-bigtable-kafka-connect-sink/src/main/java/com/google/cloud/kafka/connect/bigtable/BigtableSinkTask.java b/google-cloud-bigtable-kafka-connect-sink/src/main/java/com/google/cloud/kafka/connect/bigtable/BigtableSinkTask.java | |||
index 80bea508..3dcf2b27 100644 | |||
--- a/google-cloud-bigtable-kafka-connect-sink/src/main/java/com/google/cloud/kafka/connect/bigtable/BigtableSinkTask.java |
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.
is this file intentional?
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.
Yes, see this section in the performance tests' README (google-cloud-bigtable-kafka-connect-sink/doc/performance/README.md
):
### Tracing code
Tracing code is not present in the release code.
The code of tracing implementation is available in [`MessageTracer.java`](MessageTracer.java).
To enable tracing, put it into `sink/src/main/java/com/google/cloud/kafka/connect/bigtable/tracing` and apply [`enable_tracing.patch` git patch](enable_tracing.patch).
I think it is a better idea to show the diff rather than store the whole file since it can be more easily applied on changed file in the future to rerun the tests, but if you prefer to do it in some other way, I'll change it.
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.
Please add this extra header so we can monitor usage. That goes in ~line 538
import com.google.api.gax.rpc.FixedHeaderProvider;
...
dataStubSettings.setHeaderProvider(
FixedHeaderProvider.create("user-agent", "bigtable-java/kafka-connect-sink/" + PackageMetadata.getVersion()));
Header added to both Admin and Data clients, hopefully it wasn't overzealous. In one of your previous comments you mentioned "some junk files". There are symlink present in
I think the symlinks are a surprisingly clean solution to this problem - I think we don't really care about the integration tests on Windows and the other solutions are more complicated in my opinion. There are k8s manifests present in I think all the other (
So can I get an approval of this PR without any caveats or do I still need to do something? Apart from the Kokoro config, but it's going to happen only in the new repo, right? |
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.
LGTM!
@prawilny gotcha, I didn't realize that those were symlinked files |
This PR adds Kafka Connect sink connector.
The code is to land in a different repository, but the repository hasn't been created yet, so we bring the code for early review here.
The fact that it's targetting another repo is the reason of the following:
Things yet to be done (in future PRs):