Add tests for 1PC COPY on append and hash-distributed tables #2244

lithp · 2018-06-29T02:08:38Z

No description provided.

codecov · 2018-07-02T04:10:22Z

Codecov Report

Merging #2244 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #2244     +/-   ##
=========================================
- Coverage   93.78%   93.68%   -0.1%     
=========================================
  Files         110      106      -4     
  Lines       27576    26638    -938     
=========================================
- Hits        25861    24956    -905     
+ Misses       1715     1682     -33

onderkalaci

I think one general open question for adding the failure testing is the following:

Do we need to add tests for all possible scenarios for all tests (e.g., #2212) including
* Replication factor 1 vs 2
* Parallel vs Sequential mode (if applicable)
* 1PC vs 2PC
* Cancel tests for each step as we do for failures

If we add all the tests, we'd end up with a huge duplication since we'd be exercising the same code-paths over and over and over again. On the other hand, we're going add these tests only once and we'd be able forget about almost all network failure scenarios if we have all those tests.

What would be your thoughts on this? I'm also curious what @marcocitus would think about that.

onderkalaci · 2018-07-02T08:17:03Z

src/test/regress/expected/failure_1pc_copy_append.out

+SELECT * FROM citus.dump_network_traffic();
+ conn |   source    |                                                                                                                                                                                                                                                                                               message                                                                                                                                                                                                                                                                                               
+------+-------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+    0 | coordinator | [initial message]


I can see that this network dump is super useful and have it during development or adding new tests is very useful.

But, travis sometimes fail due to place of + 0 | coordinator | ['Terminate()'].

Some random idea to not get into this state in general: Shall we (or could we) filter only the packets that we're going to test from dump_network_traffic ()?

But, travis sometimes fail due to place of + 0 | coordinator | ['Terminate()'].

This is a bug that should be fixed. I guess it's a timing issue. mitmdump should ignore any packets from connections which existed before the last time the buffer was cleared. I can fix it tomorrow!

I've removed the packet trace for now.

onderkalaci · 2018-07-02T08:18:54Z

src/test/regress/expected/failure_1pc_copy_append.out

+CONTEXT:  while executing command on localhost:57640
+ERROR:  failure on connection marked as essential: localhost:57640
+SELECT * FROM pg_dist_shard s, pg_dist_shard_placement p
+  WHERE (s.shardid = p.shardid) AND s.logicalrelid = 'copy_test'::regclass;


Let's add ORDER BY to all queries selecting from pg_dist* to be on the safe side.

Good point, pushed a new commit where we always sort by placementid.

metdos · 2018-07-06T09:09:42Z

I think one general open question for adding the failure testing is the following:

Do we need to add tests for all possible scenarios for all tests (e.g., #2212) including

Replication factor 1 vs 2

Parallel vs Sequential mode (if applicable)

1PC vs 2PC

Cancel tests for each step as we do for failures

If we add all the tests, we'd end up with a huge duplication since we'd be exercising the same code-paths over and over and over again. On the other hand, we're going add these tests only once and we'd be able forget about almost all network failure scenarios if we have all those tests.

What would be your thoughts on this? I'm also curious what @marcocitus would think about that.

Hey @marcocitus, any ideas on this? We will start to write automated tests next week, so if you have some comments, please let us know.

marcocitus · 2018-07-06T10:06:37Z

we'd be able forget about almost all network failure scenarios

At some point adding a lot more failure tests using the current framework will have diminishing returns, while there are other areas that are not yet covered. In particular network timeouts and network failures behind a pgbouncer (e.g. BEGIN results in a query_wait_timeout error).

More on topic: The way 1PC vs. 2PC works in COPY is not different than in other parts of the code. It might make sense to dedicate a test file specifically to commit/abort failures in 1PC and 2PC, while not doing commit-time fault-injection in other tests.

Some code paths with different failure handling logic:

COPY
DDL & multi-shard DML with parallel and sequential execution
Bare DDL (vacuum, create index concurrently)
Multi-shard SELECT
Single-shard SELECT
Single-shard DML outside a transaction block
Single-shard DML in a transaction block
Multi-statement transactions that span across multiple shards
DML on reference table
Commit/abort/prepare
Begin
Savepoints

Ideally, we would test all of them with replication vs. no replication, and test cancellation. I don't think there are other dimensions we need to consider.

metdos · 2018-07-08T09:37:15Z

@lithp, could you rebase this to the master and make ready to merge?

metdos · 2018-07-08T09:37:47Z

Are these tests checked in #2262?

metdos · 2018-07-10T13:55:35Z

Just noting that other tests are blocked on this for Travis, so it would be good if prioritize this over others :)

onderkalaci · 2018-07-12T14:52:45Z

src/test/regress/expected/failure_1pc_copy_hash.out

+(4 rows)
+
+-- the COPY data fails
+SELECT citus.mitmproxy('conn.onCopyData().killall()');


Maybe add one more step

SELECT citus.mitmproxy('conn.onCommandComplete(command="COPY 4").kill()');

I don't see a lot of value in this test because, from the coordinator's POV, it looks exactly the same as the conn.onCopyData().killall() test, but I've just pushed a commit which adds it.

sumedhpathak · 2018-07-13T19:42:15Z

@lithp Could you follow-up on the action-items and close this PR out?

lithp · 2018-07-26T22:53:25Z

@onderkalaci I've rebased on master and addressed, I think, all your comments.

onderkalaci

The tests are pretty comprehensive, and all looks good, after considering two minor comments

(Also, I think you could consider merging the ephemeral port PR first and update the test outputs here).

onderkalaci · 2018-07-27T06:20:19Z

src/test/regress/expected/failure_1pc_copy_hash.out

+
+-- ==== okay, run some tests where there's only one active shard ====
+COPY copy_test FROM PROGRAM 'echo 0, 0 && echo 1, 1 && echo 2, 4 && echo 3, 9' WITH CSV;
+SELECT * FROM copy_test;


Minor: Shall we add ORDER BY? Though the data comes from a single shard, there is a possibility of ending up with a different order.

onderkalaci · 2018-07-27T06:30:15Z

src/test/regress/expected/failure_1pc_copy_append.out

+CONTEXT:  while executing command on localhost:57640
+WARNING:  connection not open
+CONTEXT:  while executing command on localhost:57640
+ERROR:  failure on connection marked as essential: localhost:57640


Pretty minor: this test took some time for me to digest. Since we've not printed placements just after the successful COPY operation 2 lines before, I thought oh, we're erroring out but also creating the placements successfully, what's happening 😄

So, could be useful to print the placements just before starting the test as well.

good point, added!

lithp requested a review from onderkalaci June 29, 2018 02:08

begriffs added the needs review label Jun 29, 2018

lithp force-pushed the mitmproxy-failure-testing branch from 3121431 to 6c01440 Compare June 29, 2018 02:13

lithp mentioned this pull request Jun 29, 2018

Mitmproxy-based automated failure testing #2119

Merged

16 tasks

lithp force-pushed the failure-1pc-copy branch from dea22c4 to c723121 Compare July 2, 2018 04:10

onderkalaci reviewed Jul 2, 2018

View reviewed changes

lithp force-pushed the mitmproxy-failure-testing branch from 6c01440 to 3e309e3 Compare July 6, 2018 18:51

This was referenced Jul 11, 2018

Added failure test for create index concurrently #2210

Merged

Add failure test for copy on hash distributed table #2273

Merged

onderkalaci reviewed Jul 12, 2018

View reviewed changes

lithp force-pushed the failure-1pc-copy branch from c723121 to bbbe9ed Compare July 26, 2018 22:28

lithp changed the base branch from mitmproxy-failure-testing to master July 26, 2018 22:28

onderkalaci approved these changes Jul 27, 2018

View reviewed changes

lithp force-pushed the failure-1pc-copy branch 3 times, most recently from 81a9493 to c23bd5c Compare July 31, 2018 21:27

lithp added 2 commits July 31, 2018 14:55

Add tests for 1PC COPY on append and hash-distributed tables

173c443

Update tests for our updated $mitmPort

ea485ab

lithp force-pushed the failure-1pc-copy branch from c23bd5c to ea485ab Compare July 31, 2018 21:57

lithp merged commit 82fa85f into master Jul 31, 2018

marcocitus deleted the failure-1pc-copy branch October 21, 2019 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for 1PC COPY on append and hash-distributed tables #2244

Add tests for 1PC COPY on append and hash-distributed tables #2244

lithp commented Jun 29, 2018

codecov bot commented Jul 2, 2018 •

edited

Loading

onderkalaci left a comment

onderkalaci Jul 2, 2018

lithp Jul 12, 2018

lithp Jul 26, 2018

onderkalaci Jul 2, 2018

lithp Jul 26, 2018

metdos commented Jul 6, 2018

marcocitus commented Jul 6, 2018

metdos commented Jul 8, 2018

metdos commented Jul 8, 2018

metdos commented Jul 10, 2018

onderkalaci Jul 12, 2018

lithp Jul 26, 2018

sumedhpathak commented Jul 13, 2018

lithp commented Jul 26, 2018

onderkalaci left a comment

onderkalaci Jul 27, 2018

lithp Jul 31, 2018

onderkalaci Jul 27, 2018

lithp Jul 31, 2018

Add tests for 1PC COPY on append and hash-distributed tables #2244

Add tests for 1PC COPY on append and hash-distributed tables #2244

Conversation

lithp commented Jun 29, 2018

codecov bot commented Jul 2, 2018 • edited Loading

Codecov Report

onderkalaci 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

metdos commented Jul 6, 2018

marcocitus commented Jul 6, 2018

metdos commented Jul 8, 2018

metdos commented Jul 8, 2018

metdos commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumedhpathak commented Jul 13, 2018

lithp commented Jul 26, 2018

onderkalaci 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

codecov bot commented Jul 2, 2018 •

edited

Loading