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

[BEAM-9359] Switch to Data Catalog client #10917

Merged
merged 2 commits into from
Feb 22, 2020

Conversation

TheNeuralBit
Copy link
Member

Use DataCatalog client, rather than using the gRPC stub directly.

CC: @kennknowles

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- Build Status

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@TheNeuralBit
Copy link
Member Author

Run SQL PostCommit

@lukecwik
Copy link
Member

Have you looked at the linkage checker for issues that swapping versions is going to cause?

It also looks like we are regressing the package namespace to v1beta1 even though we are increasing the version number.

@TheNeuralBit
Copy link
Member Author

This isn't just swapping versions, it's switching to using the java client library, rather than using the gRPC stub and protos directly.

How do I run the linkage checker?

@lukecwik
Copy link
Member

Linkage checker instructions:

// Reports linkage errors across multiple Apache Beam artifact ids.

// Reports linkage errors across multiple Apache Beam artifact ids.
//
// To use (from the root of project):
//    ./gradlew -Ppublishing -PjavaLinkageArtifactIds=artifactId1,artifactId2,... :checkJavaLinkage
//
// For example:
//    ./gradlew -Ppublishing -PjavaLinkageArtifactIds=beam-sdks-java-core,beam-sdks-java-io-jdbc :checkJavaLinkage
//
// Note that this task publishes artifacts into your local Maven repository.

@kennknowles kennknowles requested review from lukecwik and kennknowles and removed request for lukecwik February 21, 2020 05:00
Copy link
Member

@kennknowles kennknowles left a comment

Choose a reason for hiding this comment

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

Code changes LGTM pending linkage checker. Seems there's some lack of clarity around the best way to publicly depend on this functionality.

@TheNeuralBit
Copy link
Member Author

One thing I noted after testing internally is that DataCatalogClient catches and re-throws StatusRuntimeExceptions with more specific exceptions (e.g. PermissionDeniedException) I'll need to make sure we update those references.

@TheNeuralBit
Copy link
Member Author

Result of running the linkage checker script is here: https://gist.github.com/TheNeuralBit/30d4b4a851dcaeed1651cc19fedd3159

I could use some help interpreting the results.

@TheNeuralBit
Copy link
Member Author

Seems there's some lack of clarity around the best way to publicly depend on this functionality.

The GCP Data Catalog docs indicate people should be using this client library, rather than the gRPC stub.

@TheNeuralBit TheNeuralBit changed the title [WIP] Switch to Data Catalog client [BEAM-9359] Switch to Data Catalog client Feb 22, 2020
@TheNeuralBit
Copy link
Member Author

Run SQL PostCommit

@TheNeuralBit
Copy link
Member Author

I pushed a change to replace the StatueRuntimeException with specific gax exceptions. Planning on merging when tests are green.

@lukecwik
Copy link
Member

the linkage checker results are stale and are picking up warnings due to a jackson version change so it seems like no new linage issues have been reported

@TheNeuralBit TheNeuralBit merged commit 1117508 into apache:master Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants