doc(pubsub): delete schemas > 48 hours old #11862

alevenberg · 2023-06-12T15:39:05Z

This change is

coryan · 2023-06-12T15:41:20Z

google/cloud/pubsub/samples/pubsub_samples_common.cc

+    if (schema_create_time < time_now - absl::Hours(48)) {
+      google::pubsub::v1::DeleteSchemaRequest request;
+      request.set_name((*schema).name());
+      schema_admin.DeleteSchema(request);


nit: we usually write (void)schema_admin.DeleteSchema(request) to signal that we really want to ignore the return value.

coryan · 2023-06-12T15:42:39Z

google/cloud/pubsub/samples/samples.cc

@@ -2123,6 +2124,8 @@ void AutoRunAvro(

  std::cout << "\nRunning DeleteSchema() sample [avro]" << std::endl;
  DeleteSchema(schema_admin, {project_id, avro_schema_id});
+
+  CleanupSchemas(schema_admin, project_id, absl::Now());


Please move this to the beginning of the function. We want this to run first so we can recover quota space, if we ever run out, before starting the test.

coryan · 2023-06-12T15:45:21Z

google/cloud/pubsub/samples/pubsub_samples_common.cc

+    absl::Time schema_create_time =
+        absl::FromUnixSeconds((*schema).revision_create_time().seconds()) +
+        absl::Nanoseconds((*schema).revision_create_time().nanos());


We have a helper:

google-cloud-cpp/google/cloud/internal/time_utils.h

Line 42 in 654439a

absl::Time ToAbslTime(google::protobuf::Timestamp const& proto);

consider:

Suggested change

absl::Time schema_create_time =

absl::FromUnixSeconds((*schema).revision_create_time().seconds()) +

absl::Nanoseconds((*schema).revision_create_time().nanos());

auto const schema_create_time =

google::cloud::internal::ToAbslTime(schema->revision_create_time());

coryan · 2023-06-12T15:46:35Z

google/cloud/pubsub/samples/pubsub_samples_common.cc

+
+    if (schema_create_time < time_now - absl::Hours(48)) {
+      google::pubsub::v1::DeleteSchemaRequest request;
+      request.set_name((*schema).name());


nit: we spell that: schema->name()

codecov · 2023-06-12T16:05:31Z

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.01 ⚠️

Comparison is base (b3b5998) 93.67% compared to head (9e1c0d3) 93.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11862      +/-   ##
==========================================
- Coverage   93.67%   93.66%   -0.01%     
==========================================
  Files        1838     1838              
  Lines      166225   166240      +15     
==========================================
+ Hits       155704   155711       +7     
- Misses      10521    10529       +8

Impacted Files	Coverage Δ
...ogle/cloud/pubsub/samples/pubsub_samples_common.cc	`93.98% <38.46%> (-6.02%)`	⬇️
google/cloud/pubsub/samples/samples.cc	`87.15% <100.00%> (+0.07%)`	⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

google/cloud/pubsub/samples/pubsub_samples_common.cc

coryan · 2023-06-12T17:41:33Z

google/cloud/pubsub/samples/samples.cc

@@ -2125,7 +2126,8 @@ void AutoRunAvro(
  std::cout << "\nRunning DeleteSchema() sample [avro]" << std::endl;
  DeleteSchema(schema_admin, {project_id, avro_schema_id});

-  CleanupSchemas(schema_admin, project_id, absl::Now());
+  std::chrono::system_clock clock;
+  CleanupSchemas(schema_admin, project_id, absl::FromChrono(clock.now()));


nit-ish: this is a static function, you can say std::chrono::system_clock::now()

alevenberg

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @coryan and @devbww)

google/cloud/pubsub/samples/pubsub_samples_common.cc line 199 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

We have a helper:

google-cloud-cpp/google/cloud/internal/time_utils.h

Line 42 in 654439a

absl::Time ToAbslTime(google::protobuf::Timestamp const& proto);

consider:
auto const schema_create_time =
    google::cloud::internal::ToAbslTime(schema->revision_create_time());

Done.

google/cloud/pubsub/samples/pubsub_samples_common.cc line 201 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

Should we also only be considering schemas with the RandomSchemaId() prefix?

That's a good idea. Added a check.

google/cloud/pubsub/samples/samples.cc line 2128 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Please move this to the beginning of the function. We want this to run first so we can recover quota space, if we ever run out, before starting the test.

Done.

google/cloud/pubsub/samples/samples.cc line 2130 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

nit-ish: this is a static function, you can say std::chrono::system_clock::now()

The old cmake build didn't work with absl::Now as well :(

Fixed.

cmake-oldest-deps-pr was failing to recognize absl::Now, using chrono time instread

alevenberg requested a review from a team as a code owner June 12, 2023 15:39

product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Jun 12, 2023

coryan suggested changes Jun 12, 2023

View reviewed changes

devbww reviewed Jun 12, 2023

View reviewed changes

google/cloud/pubsub/samples/pubsub_samples_common.cc Show resolved Hide resolved

coryan reviewed Jun 12, 2023

View reviewed changes

coryan approved these changes Jun 12, 2023

View reviewed changes

alevenberg commented Jun 12, 2023

View reviewed changes

alevenberg changed the title ~~docs(pubsub): delete schemas > 48 hours old~~ doc(pubsub): delete schemas > 48 hours old Jun 12, 2023

alevenberg force-pushed the issue-11720-b branch from 8990703 to 584f76e Compare June 12, 2023 20:34

alevenberg added 9 commits June 13, 2023 20:11

add delete and rollback commands

6412051

add the testing for the new commands

530feea

add cleanup for schemas > 48 hours old

1f58a7d

fix merge errors

f197b17

address comments and remove absl::Now()

2fb4e44

cmake-oldest-deps-pr was failing to recognize absl::Now, using chrono time instread

add a check for the schema prefix

f6cb2b0

fix system clock error

096c5de

fix chrono clock

edb638f

fix post merge

d35027c

alevenberg force-pushed the issue-11720-b branch from 584f76e to d35027c Compare June 13, 2023 20:19

alevenberg added 2 commits June 13, 2023 21:16

fix failures

4c33339

fix merge

9e1c0d3

alevenberg merged commit 2417284 into googleapis:main Jun 14, 2023

alevenberg deleted the issue-11720-b branch June 14, 2023 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc(pubsub): delete schemas > 48 hours old #11862

doc(pubsub): delete schemas > 48 hours old #11862

alevenberg commented Jun 12, 2023 •

edited

Loading

coryan Jun 12, 2023

coryan Jun 12, 2023

coryan Jun 12, 2023

coryan Jun 12, 2023

codecov bot commented Jun 12, 2023 •

edited

Loading

coryan Jun 12, 2023

alevenberg left a comment

doc(pubsub): delete schemas > 48 hours old #11862

doc(pubsub): delete schemas > 48 hours old #11862

Conversation

alevenberg commented Jun 12, 2023 • edited Loading

coryan Jun 12, 2023

Choose a reason for hiding this comment

coryan Jun 12, 2023

Choose a reason for hiding this comment

coryan Jun 12, 2023

Choose a reason for hiding this comment

coryan Jun 12, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2023 • edited Loading

Codecov Report

coryan Jun 12, 2023

Choose a reason for hiding this comment

alevenberg left a comment

Choose a reason for hiding this comment

alevenberg commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading