[Fix Python Bigtable dataloss bug] Stop unsetting timestamps of -1 by ahmedabu98 · Pull Request #28624 · apache/beam

ahmedabu98 · 2023-09-22T19:48:22Z

ahmedabu98 · 2023-09-22T19:49:48Z

Run Python_Xlang_Gcp_Direct PostCommit

ahmedabu98 · 2023-09-22T20:07:58Z

Run Python_Xlang_Gcp_Dataflow PostCommit

igorbernstein2 · 2023-09-22T20:09:42Z

sdks/python/apache_beam/io/gcp/bigtableio.py

          }
          micros = mutation.set_cell.timestamp_micros
-          if micros > -1:
+          if micros >= -1:


why is this conditional? why isnt the timestamp just passed through?

I forgot why it was originally set up like this, but I don't see a reason for it

sdks/python/apache_beam/io/gcp/bigtableio_it_test.py

ahmedabu98 · 2023-09-22T20:49:35Z

Run Python_Xlang_Gcp_Direct PostCommit

igorbernstein2

lgtm & thanks for the quick turnaround

codecov · 2023-09-22T21:51:09Z

Codecov Report

Merging #28624 (deb221c) into master (426dbd3) will decrease coverage by 0.01%.
Report is 6 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #28624      +/-   ##
==========================================
- Coverage   72.20%   72.20%   -0.01%     
==========================================
  Files         684      684              
  Lines      101131   101130       -1     
==========================================
- Hits        73023    73021       -2     
- Misses      26529    26530       +1     
  Partials     1579     1579

Flag	Coverage Δ
python	`82.76% <ø> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
sdks/python/apache_beam/io/gcp/bigtableio.py	`74.04% <ø> (-0.59%)`	⬇️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2023-09-22T22:34:40Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @liferoad for label python.
R: @bvolpato for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

ahmedabu98 · 2023-09-22T23:16:44Z

Run Python_Xlang_Gcp_Direct PostCommit

Abacn · 2023-09-22T23:22:46Z

BigtableWriteSchemaTransform can be used in other cases (template, xlang other than python), should we set Timestamp to -1 if it is not provided in incoming row?

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableWriteSchemaTransformProvider.java

Line 183 in f635ade

if (mutation.containsKey("timestamp_micros")) {

ahmedabu98 · 2023-09-22T23:56:12Z

@Abacn I think that's a good call, will include it in this PR

ahmedabu98 · 2023-09-23T00:03:02Z

Run Python_Xlang_Gcp_Direct PostCommit

Abacn · 2023-09-23T00:40:40Z

fyi test passed on GitHub Action also: https://github.com/apache/beam/actions/runs/6280225908

Though current phrase trigger on GitHub Action is not conveniently visible (we're investigating a solution)

ahmedabu98 · 2023-09-23T01:24:58Z

Merging after Java_GCP_IO_Direct tests pass

ahmedabu98 · 2023-09-23T03:55:36Z

Failing tests are irrelevant, merging now

kennknowles · 2023-09-24T18:10:39Z

.../src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableWriteSchemaTransformProvider.java

-            }
+                        ByteString.copyFrom(ofNullable(mutation.get("family_name")).get()))
+                    // Use timestamp if provided, else default to -1 (current Bigtable server time)
+                    .setTimestampMicros(


nit: would the most appropriate logic be more like this?

if (mutation.containsKey("timestamp_micros")) { builder.setTimestampMicros(mutation.get("timestamp_micros")); }

and so on probably for all the fields, since proto handling of optional fields is not usable

Could definitely add those checks, but want to point out that the input mutation map object is constructed here:

beam/sdks/python/apache_beam/io/gcp/bigtableio.py

Lines 249 to 258 in 68cf802

for mutation in direct_row._get_mutations():

if mutation.__contains__("set_cell"):

mutation_dict = {

"type": b'SetCell',

"family_name": mutation.set_cell.family_name.encode('utf-8'),

"column_qualifier": mutation.set_cell.column_qualifier,

"value": mutation.set_cell.value,

"timestamp_micros": struct.pack(

'>q', mutation.set_cell.timestamp_micros)

}

This process is internal, so we always expect these fields to exist (at least if we're talking xlang. I'm not aware of other ways SchemaTransforms are used). Users wouldn't be constructing their own Beam Row mutations

I just mean to be a true passthrough, we pass "not present" to "not present" instead of putting the logic for choosing a default value into our code, where it is duplicated and could get out of sync.

Sure I can agree with that. I can address this nit in another PR but for now the current changes should be okay to cherry-pick?

@Abacn this means we'd have to make sure the Go wrapper defaults timestamps to time at ingestion. Also templates don't use the schematransform AFAIK so it shouldn't matter there anyways right?

@igorbernstein2 cc'ing you to this thread as well cuz I heard the Go wrapper is implemented by the Bigtable team and maybe you can weigh in on this

This is due to a different/inconsistent behavior in Java an Python API. We cannot "pass "not present" to "not present"". For Python, If timestamp not set it defaults to -1: https://github.com/googleapis/python-bigtable/blob/e5af3597f45fc4c094c59abca876374f5a866c1b/google/cloud/bigtable/row.py#L164

For Java, if timestamp not set it defaults to 0 and causing problem

Arguably the Documentation for Java client asks user to set Timestamp and warns that it will defaults to 0 if unspecified: https://github.com/googleapis/java-bigtable/blob/15cd4868ff807513914095a3758134eaa14f0ea3/proto-google-cloud-bigtable-v2/src/main/java/com/google/bigtable/v2/Mutation.java#L902

Consequently the possible misuse (did not set Timestamp and then data loss) could still happen in Java BigtableIO with user constructed Mutation: #27022

It might be a useful action item to add validation somehow to prevent unset timestamp

…#28634)

add test for unset timestamp

0c8464d

github-actions bot added python io gcp labels Sep 22, 2023

add fix and test error message

491c1ac

ahmedabu98 marked this pull request as ready for review September 22, 2023 20:08

re-enable expansion service

babff94

igorbernstein2 reviewed Sep 22, 2023

View reviewed changes

sdks/python/apache_beam/io/gcp/bigtableio_it_test.py Show resolved Hide resolved

ahmedabu98 added 2 commits September 22, 2023 16:30

pass thru all timestamps; add explicit -1 timestamp test

deb26a7

actually set test timestamp to -1

1539537

igorbernstein2 approved these changes Sep 22, 2023

View reviewed changes

github-actions bot added the Next Action: Reviewers label Sep 22, 2023

remove explicit -1 test; default to -1 in schematransform

951ba03

github-actions bot added java and removed java labels Sep 23, 2023

ahmedabu98 added this to the 2.51.0 Release milestone Sep 23, 2023

Abacn approved these changes Sep 23, 2023

View reviewed changes

fix schematransform test

74cb85e

github-actions bot added the java label Sep 23, 2023

ahmedabu98 changed the title ~~[Python Bigtable dataloss bug] Stop unsetting timestamps of -1~~ [Fix Python Bigtable dataloss bug] Stop unsetting timestamps of -1 Sep 23, 2023

github-actions bot added java and removed java labels Sep 23, 2023

ahmedabu98 added 2 commits September 22, 2023 21:41

make the test a lil stronger

29a7888

import options

deb221c

ahmedabu98 merged commit 68cf802 into apache:master Sep 23, 2023

ahmedabu98 mentioned this pull request Sep 23, 2023

CP for #28624 into release 2.51.0 (Bigtable Python timestamp bug fix) #28634

Merged

kennknowles reviewed Sep 24, 2023

View reviewed changes

kennknowles pushed a commit that referenced this pull request Sep 27, 2023

CP for #28624 into release 2.51.0 (Bigtable Python timestamp bug fix) (…

70f4a1a

…#28634)

Abacn mentioned this pull request Sep 29, 2023

[Bug]: Possible data loss in BigtableIO r/w if timestamp not set (default to epoch) #27022

Open

15 tasks

	for mutation in direct_row._get_mutations():
	if mutation.__contains__("set_cell"):
	mutation_dict = {
	"type": b'SetCell',
	"family_name": mutation.set_cell.family_name.encode('utf-8'),
	"column_qualifier": mutation.set_cell.column_qualifier,
	"value": mutation.set_cell.value,
	"timestamp_micros": struct.pack(
	'>q', mutation.set_cell.timestamp_micros)
	}

Conversation

ahmedabu98 commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahmedabu98 commented Sep 22, 2023

Uh oh!

ahmedabu98 commented Sep 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahmedabu98 commented Sep 22, 2023

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Sep 22, 2023

Uh oh!

ahmedabu98 commented Sep 22, 2023

Uh oh!

Abacn commented Sep 22, 2023

Uh oh!

ahmedabu98 commented Sep 22, 2023

Uh oh!

ahmedabu98 commented Sep 23, 2023

Uh oh!

Abacn commented Sep 23, 2023

Uh oh!

ahmedabu98 commented Sep 23, 2023

Uh oh!

ahmedabu98 commented Sep 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abacn Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahmedabu98 commented Sep 22, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading

Abacn Sep 26, 2023 •

edited

Loading