-
Notifications
You must be signed in to change notification settings - Fork 366
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
feat: add BigQueryJob library #14403
Conversation
c334d4d
to
55c7658
Compare
791d672
to
80e648d
Compare
80e648d
to
5d79418
Compare
Here is the summary of possible violations 😱 There is a possible violation for not having product prefix.
The end of the violation section. All the stuff below is FYI purposes only. Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
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 would wait for @dbolduc to review the CMake and Bazel files.
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
//! [START bigqueryjob_quickstart] [all] |
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.
Consider
//! [START bigqueryjob_quickstart] [all] | |
//! [all] |
or
//! [START bigquery_job_quickstart] [all]
std::cerr << "google::cloud::Status thrown: " << status << "\n"; | ||
return 1; | ||
} | ||
//! [END bigqueryjob_quickstart] [all] |
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.
Remember to make a matching change.
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 am not sure I agree with where we are placing the library. I think the point is that adding the jobs/
directory makes globbing easier when we introduce bigquery_admin
.
It still feels like it'd be easier for us (and our customers build scripts) to combine them into one library.
I really think we should consider google/cloud/bigquery_job/
1 with all the scaffolding that comes with it. Code-wise bigquery_job
is independent of bigquery
.
Pro:
- all of the customization goes away. i.e. the library is no different than
sql
.
Con:
- in v3, we will probably combine the bigquery libraries? If we want to move it later, we can leave forwarding headers, though.
- underscore in directory name is not cool with vcpkg. The fix is not too bad though.
Neutral:
- docs are separate
WDYT?
Footnotes
-
or
google/cloud/bigquery_rest/
↩
@@ -445,6 +445,15 @@ service { | |||
emitted_rpcs: ["ScheduleTransferRuns"] | |||
} | |||
|
|||
service { | |||
service_proto_path: "google/cloud/bigquery/v2/job.proto" | |||
product_path: "google/cloud/bigquery/job/v2" |
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.
Why not mimic the googleapis structure: google/cloud/bigquery/v2
?
It would not conflict with our google/cloud/bigquery/v2/minimal
@@ -182,6 +182,14 @@ int WriteInstallDirectories( | |||
"_mocks"); | |||
} | |||
} | |||
|
|||
// TODO(#14402): Consider enhancing the generator_config.proto to allow custom |
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 that this is awesome, but the cmake-install
script has a place to enumerate special directions. We use it for storage_grpc
:
google-cloud-cpp/ci/cloudbuild/builds/cmake-install.sh
Lines 145 to 159 in e9352ce
./lib64/cmake/google_cloud_cpp_bigquery_rest | |
./lib64/cmake/google_cloud_cpp_common | |
./lib64/cmake/google_cloud_cpp_compute_protos | |
./lib64/cmake/google_cloud_cpp_googleapis | |
./lib64/cmake/google_cloud_cpp_grafeas | |
./lib64/cmake/google_cloud_cpp_grpc_utils | |
./lib64/cmake/google_cloud_cpp_logging_type | |
./lib64/cmake/google_cloud_cpp_iam_v2 | |
./lib64/cmake/google_cloud_cpp_mocks | |
./lib64/cmake/google_cloud_cpp_oauth2 | |
./lib64/cmake/google_cloud_cpp_opentelemetry | |
./lib64/cmake/google_cloud_cpp_rest_internal | |
./lib64/cmake/google_cloud_cpp_rest_protobuf_internal | |
./lib64/cmake/google_cloud_cpp_storage_grpc | |
./lib64/cmake/google_cloud_cpp_storage_grpc_mocks |
@@ -211,6 +211,7 @@ time { | |||
|
|||
mapfile -t libraries < <(features::libraries) | |||
for library in "${libraries[@]}" opentelemetry; do | |||
[[ $library == "bigquery_job" ]] && continue |
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.
Will this library have a README?
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.
Where are we going to say how to build the BQ job library? Something needs to say "Add bigquery_job
to the GOOGLE_CLOUD_CPP_ENABLE=...
option."
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.
Storage does this:
`google-cloud-cpp::%experimental-storage_grpc` instead |
but it doesn't mention GOOGLE_CLOUD_CPP_ENABLE=...
:/
I guess we rely on:
- `experimental-storage_grpc` enables the GCS+gRPC plugin. Contact your account |
@@ -0,0 +1,37 @@ | |||
# Copyright 2024 Google LLC |
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.
Remove unused file. We use globs in the build scripts.
${proto_list} PROTO_PATH_DIRECTORIES "${EXTERNAL_GOOGLEAPIS_SOURCE}" | ||
"${PROTO_INCLUDE_DIR}") | ||
external_googleapis_set_version_and_alias(bigquery_v2_protos) | ||
target_link_libraries(google_cloud_cpp_bigquery_v2_protos PUBLIC ${proto_deps}) |
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.
just looking ahead, bigquery_admin
will need this too. We cannot recreate the target.
We will probably want to factor out a common function like:
if (NOT TARGET google_cloud_cpp_bigquery_v2_protos)
# define proto library
endif ()
@@ -198,6 +198,18 @@ function (google_cloud_cpp_add_gapic_library library display_name) | |||
endif () | |||
endforeach () | |||
|
|||
foreach (dir IN LISTS _opt_ADDITIONAL_DOXYGEN_DIRS) |
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.
So the point of this is for bigquery_job
docs to show up under cloud.google.com/cpp/docs/reference/bigquery/latest
Do the docs look okay?
e.g. we won't have a bigquery_v2_protos
here. I am not sure what that results in.
"google-cloud-cpp::${library}_protos") |
#include "google/cloud/bigquery/job/v2/job_client.h" | ||
#include <iostream> | ||
|
||
namespace {} // namespace |
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.
huh?
|
||
bigquery_proto::ListJobsRequest list_request; | ||
list_request.set_project_id(project_id); | ||
std::vector<std::string> job_ids; |
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.
unused? We must not be running clang-tidy on this
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 you want to add bigquery_job
here:
google-cloud-cpp/ci/lib/shard.sh
Lines 41 to 44 in 54274c0
readonly BIGQUERY_SHARD=( | |
bigquery | |
experimental-bigquery_rest | |
) |
@@ -30,3 +30,10 @@ endif () | |||
# Once the bigquery_client package is found, define new targets. | |||
add_executable(quickstart quickstart.cc) | |||
target_link_libraries(quickstart google-cloud-cpp::bigquery) | |||
|
|||
find_package(google_cloud_cpp_bigquery_job) |
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.
find_package(google_cloud_cpp_bigquery_job CONFIG)
I think we should consider google/cloud/bigquery_foo/
As we need to keep
bigquery_job
separate from the existingbigquery
target to avoid introducing REST dependencies, this required some massaging of our ci builds and generated inputs into those builds.This PR is split into several commits to group related changes for easier reviewing.
This change is