You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/12/03 03:14:27 UTC

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5345

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................

KUDU-1753 [delete_table-test] deleted-while-scanned test

Added an integration test to ensure a tablet server keeps all the
necessary data around untill a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
---
M src/kudu/integration-tests/delete_table-test.cc
1 file changed, 97 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/5345/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 6: Code-Review+1

(5 comments)

I will defer +2 to David, LGTM, few nits.

http://gerrit.cloudera.org:8080/#/c/5345/2/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

Line 1320:     // Setup batch size to be small enough to guarantee the scan will not fetch
> Another piece of information which helps to understand that impala was issu
I see, thank you for explaining.


PS2, Line 1324:     ASSERT_TRUE(scanner.HasMoreRows());
              :     KuduScanBatch batch;
              :     ASSERT_OK(scanner.NextBatch(&batch));
              :     size_t row_count = batch.NumRows();
              : 
> I didn't find better alternative for this simple check among those methods.
Since you are checking for existence of tablets on the cluster, and not specific TS, this seems fine. Personally, I would have preferred to go to master with RPC for eg, itest::GetTableLocations() or something like that because it fully reflects the table's state on the cluster. inspect_ reflects only on-disk contents on the servers, but the cleanup may not be complete by then.


http://gerrit.cloudera.org:8080/#/c/5345/6/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS6, Line 1316: shared_ptr
Nit: we could use sp::shared_ptr here to indicate that it's not C++ shared_ptr and keep "shared_ptr" reserved for C++ standard library one.


PS6, Line 1331: ON_ERROR_DO_NOT_DUMP_STACKS
Do we want to put a comment here as to why we wanted to avoid dumping stacks ?


PS6, Line 1357: StopCluster
Nit: Wasn't AssertNoCrashes above sufficient ? Or is this added as a good practice ?


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dinesh Bhat, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5345

to look at the new patch set (#7).

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................

KUDU-1753 [delete_table-test] deleted-while-in-scan test

Added an integration test to ensure a tablet server keeps all the
necessary data around until a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
3 files changed, 137 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/5345/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


KUDU-1753 [delete_table-test] deleted-while-in-scan test

Added an integration test to ensure a tablet server keeps all the
necessary data around until a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Reviewed-on: http://gerrit.cloudera.org:8080/5345
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
3 files changed, 182 insertions(+), 32 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 6:

(1 comment)

Consider +1 from my side, my browser is playing some tricks with me not showing those radio buttons at the moment.

http://gerrit.cloudera.org:8080/#/c/5345/6/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS6, Line 1316: shared_ptr
> I'm not sure I understand what you mean.
What I meant was if you see external_mini_cluster.cc, it uses both client::sp::shared_ptr and std::shared_ptr. So my suggestion was to keep the naked "shared_ptr" for std::shared_ptr and prefix other one with client::sp::(for the std::tr1:: cases). Yeah, with LIBCPP shared_ptr is same in both cases, so feel free to ignore this comment.


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5345/6/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS6, Line 1316: shared_ptr
> Nit: we could use sp::shared_ptr here to indicate that it's not C++ shared_
I'm not sure I understand what you mean.

There is

using kudu::client::sp::shared_ptr;

in the beginning of the file.  If there were an ambiguity, the compiler would give an error on this.

Besides, under the hood, that _is_ shared_ptr from the STL library.  In one case it's std::tr1::shared_ptr (for older compilers), in another it's just std::shared_ptr from the newer STL library.


PS6, Line 1331: ON_ERROR_DO_NOT_DUMP_STACKS
> Do we want to put a comment here as to why we wanted to avoid dumping stack
If would better consider adding comments why it's necessary to dump stacks in other places :)

The reason is simple: this is not the test which is exploring internals of the DeleteTable() mechanics.  If DeleteTable() is broken for some reason, that's not the test to diagnose that sort of the breakage: there are other tests for that in this file.

I'll add the comment.


PS6, Line 1357: StopCluster
> Nit: Wasn't AssertNoCrashes above sufficient ? Or is this added as a good p
AssertNoCrashes() it not enough: it's necessary to shutdown the cluster because it's started again by the next cycle of the for() loop.  Usually, StopCluster() is performed by TearDown() method of the test class, but this is not the case.  If this test is re-written using parameterized test fixtures instead of the for() loop, it would not be necessary.


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/5345/1/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS1, Line 1280: 65536 : 1024;
this is weird.
- Magic numbers? use constants?
- Is this a count or the number of rows? I thought it was a count at first but then I saw that you use this in SetBatchSizeBytes


PS1, Line 1285: w.set_table_name("table-to-delete");
if you don't care about this don't set it


PS1, Line 1286: // Actually, need just one tablet to make sure the test exercises
              :     // the scenario of deleting a tablet when fetching data from it,
              :     // not when opening the next tablet to fetch data from. However, 2 is the
              :     // minimum possible for TestWorkload. Setting this to 2  gives the desired
              :     // result anyway: the table is split-partitioned evenly across the key space
              :     // and the test inserts just a few thousand rows -- all rows are
              :     // in the first partition.
if you don't call set_num_tablets the default is 1 (not sure why you wouldn't be allowed to set though)


PS1, Line 1294: w.set_write_pattern(TestWorkload::INSERT_SEQUENTIAL_ROWS);
do you care that the rows are sequential? random would be best. also it would be good to make sure that there are flushes/compactions going on (i.e if you insert all these rows into the memrowset who's to say that we actually hang on to the blocks of diskrowsets?)


PS1, Line 1300: 8
hum that's a funny number. why 8? (not saying its "wrong" just curious why you picked this one)


PS1, Line 1311: rows_to_insert / 4
i don't understand this.


PS1, Line 1318: Now, 
remove "Now" (I've done this a lot in the past and have been told to remove by the native speakers :))


PS1, Line 1322: advertized
typo


PS1, Line 1324: vector<string> tablets;
              :     do {
              :       SleepFor(MonoDelta::FromMilliseconds(256));
              :       tablets = inspect_->ListTablets();
              :     } w
what dinesh said about the utils


PS1, Line 1341: scanner.Close();
is there a specific need to close? it'll be closed() on scope exit iirc


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5345/7/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS7, Line 1329:  // Once the first batch of data has been fetched and there is some more
              :     // to fetch, delete the table. On the error, do not dump stacks: this
              :     // assumes DeleteTable() works; if not, that's not the test which targeted
              :     // to troubleshooting DeleteTable(), there are tests for that in this file.
              :     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
> hum, this is weird. your saying that you are ignoring delete table errors b
I'm not saying errors are about to be ignored.  I'm saying that stacks would not be dumped in case of an error -- that's the comment I added by Dinesh's request.  But it seems the comment adds more confusion than clarity.  Probably, I should remove the comment.

There are couple of checks to make sure the table is deleted: check in the code below.  If you think it's necessary to add some other way to check the table is deleted -- please let me know.

Yes, I looped the test -- it fails 100% at ASSERT_TRUE(s.ok()) << s.ToString(); around at line 1348.


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5345

to look at the new patch set (#3).

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................

KUDU-1753 [delete_table-test] deleted-while-scanned test

Added an integration test to ensure a tablet server keeps all the
necessary data around untill a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
---
M src/kudu/integration-tests/delete_table-test.cc
1 file changed, 99 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/5345/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5345/1//COMMIT_MSG
Commit Message:

PS1, Line 10: untill
> typo
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 8: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 1:

(10 comments)

Thank you for the review.

http://gerrit.cloudera.org:8080/#/c/5345/1/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS1, Line 1280: 65536 : 1024;
> this is weird.
That's powers of 2.  Is that weird?

That's the only place they are used, what is the use for another constants?  The only constant which is used is later is rows_to_insert, and this is where it's defined.

I used rows_to_insert / 4 to get number of bytes which would guarantee that those rows would not be fetched at once.  I could be just rows_to_insert as number of bytes, since one rows takes more than one byte, right?


PS1, Line 1285: w.set_table_name("table-to-delete");
> if you don't care about this don't set it
Done


PS1, Line 1286: // Actually, need just one tablet to make sure the test exercises
              :     // the scenario of deleting a tablet when fetching data from it,
              :     // not when opening the next tablet to fetch data from. However, 2 is the
              :     // minimum possible for TestWorkload. Setting this to 2  gives the desired
              :     // result anyway: the table is split-partitioned evenly across the key space
              :     // and the test inserts just a few thousand rows -- all rows are
              :     // in the first partition.
> if you don't call set_num_tablets the default is 1 (not sure why you wouldn
Great: I fixed the issue with set_num_tablets() and sent it for review:

http://gerrit.cloudera.org:8080/5347


PS1, Line 1294: w.set_write_pattern(TestWorkload::INSERT_SEQUENTIAL_ROWS);
> do you care that the rows are sequential? random would be best. also it wou
Nope, I don't care much, but I just wanted to make sure they are in the same tablet.  Since I could not set number of tablets to 1 explicitly before, I assumed 2 is the minimum and that's why I used sequential.

Now, once the issues with num_tablets is resolved, I can use random.  BTW, why random is the best?

Another question: how to make sure that flushes/compactions are there?


PS1, Line 1300: 8
> hum that's a funny number. why 8? (not saying its "wrong" just curious why 
2^3

Which one would you prefer instead? :)


PS1, Line 1311: rows_to_insert / 4
> i don't understand this.
The idea is to have some number in bytes which would guarantee there would be more than one batch while fetching all rows.  It might be just rows_to_insert, since every row takes more than 1 bytes, right?  I put 4 just in case, but I think it make sense to leave just rows_to_insert -- will update.

BTW, it seems this SetBatchSizeBytes() has no effect at all -- regardless of the setting, the scanner always output batches of 100 rows.  Probably, that's another TODO.  Will file JIRA for that.


PS1, Line 1318: Now, 
> remove "Now" (I've done this a lot in the past and have been told to remove
Done


PS1, Line 1322: advertized
> typo
Done


PS1, Line 1324: vector<string> tablets;
              :     do {
              :       SleepFor(MonoDelta::FromMilliseconds(256));
              :       tablets = inspect_->ListTablets();
              :     } w
> what dinesh said about the utils
I could not find proper function without much of boilerplate code to write around.  I don't need those tablet server identifiers, etc.

Which exact function would you recommend?

Or it's better to add a new one (which incorporated this piece of code)?


PS1, Line 1341: scanner.Close();
> is there a specific need to close? it'll be closed() on scope exit iirc
Yes, it's just to make sure we don't hold any stuff open on the server.  Or it's already so by the end of the scan?


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5345/7/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS7, Line 1329:  // Once the first batch of data has been fetched and there is some more
              :     // to fetch, delete the table. On the error, do not dump stacks: this
              :     // assumes DeleteTable() works; if not, that's not the test which targeted
              :     // to troubleshooting DeleteTable(), there are tests for that in this file.
              :     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
> cool, just a matter of rephrasing/deleting the comment then
oh and I meant loop with the fix


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5345

to look at the new patch set (#4).

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................

KUDU-1753 [delete_table-test] deleted-while-scanned test

Added an integration test to ensure a tablet server keeps all the
necessary data around until a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
---
M src/kudu/integration-tests/delete_table-test.cc
1 file changed, 105 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/5345/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5345/2/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

Line 1320:     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
> Checking another server is only for so-called 'fault-tolerant' scans: ORDER
Another piece of information which helps to understand that impala was issuing non fault-tolerant requests: look at the place from where the error message where originated:

W1122 17:53:23.173130  9488 client.cc:1304] Scan at tablet server 8e6f4cf16c544ef3b7913f6cda1bfe54 (kudu-stress-6.vpc.cloudera.com:7050) of tablet impala::tpch_10_kudu.lineitem:  failed: Illegal state: Tablet is not running

So, looking around client.cc:1304 gives another useful hint: if those were fault-tolerant requests, then in impala logs there would be also messages like

'Attempting to retry scan of tablet ...'

around those 'Scan at tablet server ...', but they are absent.  So, from this it's clear the problem happened with not fault-tolerant scans.


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5345/1//COMMIT_MSG
Commit Message:

PS1, Line 10: untill
typo


http://gerrit.cloudera.org:8080/#/c/5345/2/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

Line 1320:     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
Hmmm, I haven't looked at scan codepaths, but at high level I would guess deleting a tablet while scan is in progress could be a different reaction from a client than deleting a table altogether, former one could go through retry on different tablet server, etc ? This may not give the intended test coverage for KUDU-1753 if that happened to be true.


PS2, Line 1324:     vector<string> tablets;
              :     do {
              :       SleepFor(MonoDelta::FromMilliseconds(256));
              :       tablets = inspect_->ListTablets();
              :     } while (!tablets.empty());
There are bunch of utils in cluster_itest_util.cc, you might want to use readily available routines like WaitForNumTabletsOnTS or WaitForTabletDataStateOnTS.


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 7:

> (1 comment)

ok, will remove the comment -- after couple of readings it looks like many words about nothing :)

-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5345

to look at the new patch set (#5).

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................

KUDU-1753 [delete_table-test] deleted-while-scanned test

Added an integration test to ensure a tablet server keeps all the
necessary data around until a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
2 files changed, 132 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/5345/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5345/7/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS7, Line 1329:  // Once the first batch of data has been fetched and there is some more
              :     // to fetch, delete the table. On the error, do not dump stacks: this
              :     // assumes DeleteTable() works; if not, that's not the test which targeted
              :     // to troubleshooting DeleteTable(), there are tests for that in this file.
              :     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
hum, this is weird. your saying that you are ignoring delete table errors but then you're waiting forever for the table to be deleted. maybe this is just a matter of rephrasing the comment ? are you sure that the table is going to be deleted at this point?

did you loop this test a bunch?


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 7:

> (1 comment)
 > 
 > Consider +1 from my side, my browser is playing some tricks with me
 > not showing those radio buttons at the moment.

I see -- thanks for the explanation.  I'll remove using and add client::sp  prefix there.

-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5345/2/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

Line 1320:     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
> Hmmm, I haven't looked at scan codepaths, but at high level I would guess d
Checking another server is only for so-called 'fault-tolerant' scans: ORDERED scans or scans explicitly marked fault-tolerant with KuduScanner::SetFaultTolerant() method. Other scans are not retried at other servers.

So, this test provide necessary coverage, as I understand.


PS2, Line 1324:     vector<string> tablets;
              :     do {
              :       SleepFor(MonoDelta::FromMilliseconds(256));
              :       tablets = inspect_->ListTablets();
              :     } while (!tablets.empty());
> There are bunch of utils in cluster_itest_util.cc, you might want to use re
I didn't find better alternative for this simple check among those methods.  What exact method you could recommend?


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5345/7/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS7, Line 1329:  // Once the first batch of data has been fetched and there is some more
              :     // to fetch, delete the table. On the error, do not dump stacks: this
              :     // assumes DeleteTable() works; if not, that's not the test which targeted
              :     // to troubleshooting DeleteTable(), there are tests for that in this file.
              :     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
> I'm not saying errors are about to be ignored.  I'm saying that stacks woul
cool, just a matter of rephrasing/deleting the comment then


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5345/7/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

PS7, Line 1329:  // Once the first batch of data has been fetched and there is some more
              :     // to fetch, delete the table. On the error, do not dump stacks: this
              :     // assumes DeleteTable() works; if not, that's not the test which targeted
              :     // to troubleshooting DeleteTable(), there are tests for that in this file.
              :     NO_FATALS(DeleteTable(w.table_name(), ON_ERROR_DO_NOT_DUMP_STACKS));
> oh and I meant loop with the fix
Sure -- the test with the fix worked 100% while running it ~1K times at ve0518.halxg.cloudera.com in debug and release configurations (with and without additional --stress_cpu_threads=16 option).

However, if update the test to run against 1M records, then it starts faltering in READ_LATEST mode -- the row count of the scan does not match the number of inserted rows (off a little bit).  But that's only for READ_LATEST.  Is that expected?


-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-in-scan test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dinesh Bhat, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5345

to look at the new patch set (#8).

Change subject: KUDU-1753 [delete_table-test] deleted-while-in-scan test
......................................................................

KUDU-1753 [delete_table-test] deleted-while-in-scan test

Added an integration test to ensure a tablet server keeps all the
necessary data around until a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
3 files changed, 182 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/5345/8
-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1753 [delete table-test] deleted-while-scanned test

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5345

to look at the new patch set (#6).

Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test
......................................................................

KUDU-1753 [delete_table-test] deleted-while-scanned test

Added an integration test to ensure a tablet server keeps all the
necessary data around until a scan operation ends even if tablet
is being deleted concurrently.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-1753 Impala query fails: Unable to advance iterator:
  Illegal state: Tablet is not running

Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
3 files changed, 134 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/5345/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot