TDL-24162 Log based inclusivity updates #90

bhtowles · 2023-09-28T23:05:27Z

Uncomment all record count assertions, fix with pk count where needed, new method

Description of change

(write a short description here or paste a link to JIRA)

QA steps

automated tests passing
manual qa steps passing (list below)

Risks

Rollback steps

revert this branch

…, new method

bhtowles · 2023-09-28T23:08:19Z

Why do we still have test_sync_fully.py in the repo? Can I delete it?

tests/test_sync_logical_pks.py

tests/test_log_based_interruped_replication.py

tests/base.py

…ome line length cleanup

HarrisonMarcRose

I have a couple of questions

tests/base.py

HarrisonMarcRose · 2023-10-03T18:26:17Z

tests/test_log_based_interruped_replication.py

+        expected_count['log_based_interruptible_dbo_int_and_bool_data'] = 2
+        expected_count['log_based_interruptible_dbo_int_data'] = 14


I don't see what changed to make this expectation to change? What am I missing?

tests/test_sync_logical_pks.py

tests/test_sync_full.py

tests/test_sync_logical_pks.py

…les to fix dupes

luandy64 · 2023-10-05T17:34:48Z

tests/base.py

+            stream_pks = {tuple(m.get('data', {}).get(pk) for pk in primary_keys)
+                          for m in recs['messages']
+                          if m['action'] == 'upsert'}
+
+            # remove any failed get() entries from the set to correct pk count
+            stream_pks.difference(set(tuple(None for pk in primary_keys)))
+
+            pk_count_by_stream[strm] = len(stream_pks)


Similar test code to before

primary_keys = ["pk1", "pk2"] recs = { "messages": [ { "action": "upsert", "data":{"pk1": "a", "pk2": "2",} }, { "action": "upsert", "data":{"pk1": "a", "pk2": "2",} }, { "action": "upsert", "data":{"pk1": "a", "pk2": "2",} }, { "action": "upsert", "data":{"pk1": "a", "pk2": "3",} }, { "action": "upsert", "data":{"pk1": "a",} }, { "action": "upsert", "data":{"pk1": "a", "pk2": None,} }, ] } stream_pks = {tuple(m.get('data', {}).get(pk) for pk in primary_keys) for m in recs['messages'] if m['action'] == 'upsert'} print(f"before difference: {stream_pks}") stream_pks.difference(set(tuple(None for pk in primary_keys))) print(f"after difference: {stream_pks}") print(f"Got {len(stream_pks)} unique pks")

Results:

before difference: {('a', None), ('a', '3'), ('a', '2')} after difference: {('a', None), ('a', '3'), ('a', '2')} Got 3 unique pks

set().difference() doesn't modify the set its called on

But I'm not sure I understand # remove any failed get() entries from the set to correct pk count anyway

Seems like the test needs to fail if any PK returns null

The idea was to filter out any bad upserts, we carded out an upstream verification but I do like just failing the test if we find a bad upsert here. Commit incomming.

luandy64

I left comments, but they're all for formatting things I don't care to follow up on

luandy64 · 2023-10-16T21:29:19Z

tests/test_sync_logical_pks.py

+from database import create_database, create_table, create_view, delete_by_pk, \
+    drop_all_user_databases, enable_database_tracking, insert, mssql_cursor_context_manager, \
+    update_by_pk


Looks like this was only alphabetized? I don't see any additions or deletions?

luandy64 · 2023-10-16T21:30:06Z

tests/test_sync_logical_pks.py

@@ -5,9 +5,9 @@

 from tap_tester import menagerie, runner, LOGGER


But this wasn't alphabetized too?

In general I tend to only alphabetize things when the list gets long enough that looking for an item isn't trivial. If it spans two lines or has a bunch of small values in random order for example.

luandy64 · 2023-10-16T21:30:46Z

tests/test_sync_logical_pks.py

+                    'selected-by-default': True,
+                    'inclusion': 'automatic'}},
+                {'info': {
+                    'sql-datatype': 'int', 'selected-by-default': True, 'inclusion': 'available'}}],


Any reason why this was left condensed?

luandy64 · 2023-10-16T21:31:24Z

tests/test_sync_logical_pks.py

+                {'pk': {
+                    'sql-datatype': 'int', 'selected-by-default': True, 'inclusion': 'automatic'}},
+                {'data': {
+                    'sql-datatype': 'int', 'selected-by-default': True, 'inclusion': 'available'}}],


Or why these are condensed too?

All line length

luandy64 · 2023-10-16T21:32:04Z

tests/test_sync_logical_pks.py

+                {'pk': {
+                    'sql-datatype': 'int', 'selected-by-default': True, 'inclusion': 'automatic'}},
+                {'data': {
+                    'sql-datatype': 'int', 'selected-by-default': True, 'inclusion': 'available'}}],


Seems like we like it condensed, so maybe that first one is the sore thumb and needs to change

luandy64 · 2023-10-16T21:34:19Z

tests/test_sync_logical_pks.py

                            else:
-                                # the row wasn't deleted so we can either not pass the column or it can be None
+                                # row wasn't deleted so dont pass the column or let it be None


Suggested change

# row wasn't deleted so dont pass the column or let it be None

# row wasn't deleted so don't pass the column or let it be None

Uncomment all record count assertions, fix with pk count where needed…

af0b0fb

…, new method

bhtowles added the testing QA work. No src code changes. label Sep 28, 2023

bhtowles requested review from HarrisonMarcRose, JYOTHINARAYANSETTY and bhuvana-talend September 28, 2023 23:05

bhtowles commented Sep 29, 2023

View reviewed changes

tests/test_sync_logical_pks.py Outdated Show resolved Hide resolved

tests/test_log_based_interruped_replication.py Outdated Show resolved Hide resolved

luandy64 reviewed Sep 29, 2023

View reviewed changes

tests/base.py Outdated Show resolved Hide resolved

luandy64 reviewed Sep 29, 2023

View reviewed changes

tests/base.py Outdated Show resolved Hide resolved

luandy64 reviewed Sep 29, 2023

View reviewed changes

tests/base.py Outdated Show resolved Hide resolved

bhtowles added 2 commits September 29, 2023 23:10

First round review comments (set comprehension, get() fallback) and s…

fb2bcbc

…ome line length cleanup

Delete commented out test_sync_full.py

4cb991f

HarrisonMarcRose reviewed Oct 3, 2023

View reviewed changes

luandy64 reviewed Oct 4, 2023

View reviewed changes

tests/test_sync_logical_pks.py Outdated Show resolved Hide resolved

tests/test_sync_logical_pks.py Outdated Show resolved Hide resolved

bhtowles added 2 commits October 4, 2023 22:25

Review comments 2, make pk count method generic and update to use tup…

ab98f5d

…les to fix dupes

Update log based int test to add new table pk to expected metadata

844329b

luandy64 reviewed Oct 5, 2023

View reviewed changes

bhtowles added 2 commits October 5, 2023 22:59

Review comments 3, fail test if upsert format or value is incorrect

6100490

Fix typo / bug for pk tuple iteration

7b71ef5

luandy64 approved these changes Oct 16, 2023

View reviewed changes

update comment

0674d7b

bhtowles merged commit a4c579f into master Oct 16, 2023
2 checks passed

bhtowles deleted the qa/TDL-24162-log-based-inclusivity branch October 16, 2023 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-24162 Log based inclusivity updates #90

TDL-24162 Log based inclusivity updates #90

bhtowles commented Sep 28, 2023

bhtowles commented Sep 28, 2023

HarrisonMarcRose left a comment

HarrisonMarcRose Oct 3, 2023

luandy64 Oct 5, 2023

bhtowles Oct 5, 2023

luandy64 left a comment

luandy64 Oct 16, 2023

luandy64 Oct 16, 2023

bhtowles Oct 16, 2023

luandy64 Oct 16, 2023

luandy64 Oct 16, 2023

bhtowles Oct 16, 2023

luandy64 Oct 16, 2023

luandy64 Oct 16, 2023

bhtowles Oct 16, 2023

		expected_count['log_based_interruptible_dbo_int_and_bool_data'] = 2
		expected_count['log_based_interruptible_dbo_int_data'] = 14

		@@ -5,9 +5,9 @@

		from tap_tester import menagerie, runner, LOGGER

	# row wasn't deleted so dont pass the column or let it be None
	# row wasn't deleted so don't pass the column or let it be None

TDL-24162 Log based inclusivity updates #90

TDL-24162 Log based inclusivity updates #90

Conversation

bhtowles commented Sep 28, 2023

Description of change

QA steps

Risks

Rollback steps

bhtowles commented Sep 28, 2023

HarrisonMarcRose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luandy64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment