You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/11/08 08:49:29 UTC
[kudu-CR] Add snapshot scans to fuzz-itest
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4996
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at timestamp.
When generating random operations scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save state at those (and only those)
timestamps while running.
This would fail very often before the REINSERT patches but
passes with it.
WIPish: This is just missing some dist-test runs but should
otherwise be ready for review.
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 192 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/1
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 347: CHECK_OK
nit: In the context of this method and call sites, does it make sense to use ASSERT_OK() / RETURN_NOT_OK() paradigm instead of CHECK_OK()? Since that's an internal test utility, I would expect it to fail gracefully if an error happens.
PS12, Line 533: std::greater<int>()
I don't think using greater<> instead of default less<> brings any benefit here: internally, the std::unique call does the heavy-lifting via shifting the container elements, and that's done regardless of the sort order.
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 347: CHECK_OK
> nit: In the context of this method and call sites, does it make sense to us
was mimicking the methods above, but you're right that this one returns void so we might as well change it to ASSERT_OK
PS12, Line 533: std::greater<int>()
> I don't think using greater<> instead of default less<> brings any benefit
we pop from the back later, would prefer not to change to std::list
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 8: Verified+1
Unrelated flake
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> you mean in this test? how could it be different?
Thank you for the clarification.
My question was more about generic approach to justify usage of the down_cast<> code: to use that down_cast<> it's better to be 100% sure the release mode builds will see the same control paths that debug mode builds see and all those path are validated. I don't know how to get 100% assurance on that in general. Frankly, I don't think it's even possible to provide such a guarantee. That's why I think using down_cast<> is dangerous. :)
As for this test, since it's scope is confined, I don't feel strong against using down_cast<> -- it's all up to you. I just wanted to understand what was the rationale behind using down_cast<>
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at a timestamp.
When generating random operations, scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save the state immediately before those
(and only those) timestamps.
This would fail very often before the REINSERT patches but
passes with it.
Ran 500 loops of fuzz-itest in dist-test in ASAN with:
- KUDU_ALLOW_SLOW_TESTS=1
- --stress_cpu_threads=4
All tests passed. Result:
http://dist-test.cloudera.org//job?job_id=david.alves.1480096588.30319
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Reviewed-on: http://gerrit.cloudera.org:8080/4996
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 206 insertions(+), 48 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/tablet/key_value_test_schema.h
File src/kudu/tablet/key_value_test_schema.h:
PS12, Line 47: string ret = strings::Substitute("{$0,", key);
: if (val == boost::none) {
: ret.append("NULL}");
: } else {
: ret.append(strings::Substitute("$0}", *val));
: }
: return ret;
> Done
actually I remebered I tried this before. substitute doesn't like trenary ifs when the returned type is different.
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> Ah, I see -- so it's a code style thing.
well if you look at the down_cast definition in casts.h you'll see that in debug mode it does a dynamic_cast first and makes sure that the result not null and only after that does the static_cast. so, if we always do all builds, what would be the problem here?
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4996
to look at the new patch set (#14).
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at a timestamp.
When generating random operations, scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save the state immediately before those
(and only those) timestamps.
This would fail very often before the REINSERT patches but
passes with it.
Ran 500 loops of fuzz-itest in dist-test in ASAN with:
- KUDU_ALLOW_SLOW_TESTS=1
- --stress_cpu_threads=4
All tests passed. Result:
http://dist-test.cloudera.org//job?job_id=david.alves.1480096588.30319
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 206 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/14
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4996
to look at the new patch set (#11).
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at a timestamp.
When generating random operations, scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save the state immediately before those
(and only those) timestamps.
This would fail very often before the REINSERT patches but
passes with it.
Ran 500 loops of fuzz-itest in dist-test in ASAN with:
- KUDU_ALLOW_SLOW_TESTS=1
- --stress_cpu_threads=4
All tests passed. Result:
http://dist-test.cloudera.org//job?job_id=david.alves.1480096588.30319
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 200 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/11
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 10:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/4996/10//COMMIT_MSG
Commit Message:
PS10, Line 12: operations
nit: comma after 'operations'
Line 21: passes with it.
did you loop it a bunch, particularly in an ASAN build?
http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS10, Line 289: int timestamp,
: vector<ExpectedKeyValueRow> found,
: list<string>* errors
can you doc these parameters? not obvious from the names what's going on
PS10, Line 365: found
std::move this?
Line 372: CHECK(false) << final_error;
LOG(FATAL)?
PS10, Line 529: // Sort the scan timestamps in reverse order so that we can keep popping from the back and remove
: // duplicates.
: sort(timestamps_to_scan.begin(), timestamps_to_scan.end(), std::greater<int>());
: timestamps_to_scan.erase(unique(timestamps_to_scan.begin(),
: timestamps_to_scan.end()),
: timestamps_to_scan.end() );
why not just use a std::set? this stuff isn't perf-critical so I think std::set and a ContainsKey() check is easier to read
Line 576: sort(sorted_cur_val.begin(), sorted_cur_val.end());
not quite sure what the sort is accomplishing here. Is it to move all of the boost::none values to the front of the list? aside from that, aren't all of the rows already in sorted order because the indexes in this vector are the keys themselves? std::erase_if might be more obvious what's going on
http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/tablet/key_value_test_schema.h
File src/kudu/tablet/key_value_test_schema.h:
PS10, Line 46: bool operator < (const ExpectedKeyValueRow& other) const {
: return (key < other.key);
: }
:
: bool operator > (const ExpectedKeyValueRow& other) const {
: return (key > other.key);
: }
:
:
a bit weird that these are not consistent with operator== above. I think you're just using this for the std::sort() call in the new test - could you just pass a lambda to std::sort that does the comparison you're looking for?
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 16: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> How do you guarantee that all path which you code will see in release mode
you mean in this test? how could it be different?
In any case if you look throughout the whole code base this is _always_ how we do it. there isn't a single instance of dynamic_cast and 16 instances of down_cast.
I remain unconvinced that this test, in which we choose the clock implementation on start up, should be the exception. Particularly as if you change the clock implementation on set up and not the the down_cast the build would fail.
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> I don't mind using down_cast<> here, but what is the rational behind not us
thats a google style recommendation. we also build the clock inside the test and this wouldn't work with the hybrid clock so what would be the danger here?
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4996/8//COMMIT_MSG
Commit Message:
Line 23: WIPish: This is just missing some dist-test runs but should
> Still the case?
nah, I ran a dist-test a while ago. thanks for the reminder.
http://gerrit.cloudera.org:8080/#/c/4996/8/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
Line 575: LOG(INFO) << "Cur val before: " << cur_val[test_op.val];
> Did you want to keep those LOGs?
Done
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> thats a google style recommendation. we also build the clock inside the tes
Ah, I see -- so it's a code style thing.
The danger of the static_cast<> while doing downcasting: it does not allow to see that the operand could not be downcasted to the expected type. It's possible to end-up in a situation when the code executes some other methods, not what you expect. The comment for that down_cast<> method does not make any sense to me at all, since I don't understand how one can guarantee that all the paths which the code is about to execute are covered with the debug mode runs. So, I would either forbid any downcasting at all or allow it and make sure it's done with dynamic_cast<> only, so it's safe.
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> we shouldn't use dynamic_cast on non-debug builds
I don't mind using down_cast<> here, but what is the rational behind not using dynamic_cast in non-debug builds?
As I can see, the down_cast<> is way more dangerous than dynamic_cast<> -- it's reduced just to static_cast<> in non-debug builds, which could lead to completely unpredictable results, way worse than a penalty of doing some RTTI-related calls. Also, this is just test code, so I wouldn't expect much of performance optimizations to be in place.
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4996/8//COMMIT_MSG
Commit Message:
Line 23: WIPish: This is just missing some dist-test runs but should
Still the case?
http://gerrit.cloudera.org:8080/#/c/4996/8/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
Line 575: LOG(INFO) << "Cur val before: " << cur_val[test_op.val];
Did you want to keep those LOGs?
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 292: found
> nit: consider renaming it into something more tied to the scope of this met
Done
PS12, Line 298: for (auto& found_row : found) {
: errors->push_back(Substitute("Found unexpected row: $0", found_row.ToString()));
: }
> nit: there is a piece of syntactic sugar for this, if you wish:
I like the incantation but I think its less readable. also strictly its more chars so not so sugary :)
PS12, Line 303: (*iter).
> nit: using 'iter->first' instead could save 2 characters and easier to read
Done
PS12, Line 304: vector<optional<ExpectedKeyValueRow>>
> nit: could 'auto' work here as well?
Done
PS12, Line 319:
> nit: misaligned indent
Done
PS12, Line 324:
> nit: misaligned indent
Done
PS12, Line 331:
> nit: misaligned indent
Done
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> nit: on the note of failing gracefully in case of unexpected things in the
we shouldn't use dynamic_cast on non-debug builds
PS12, Line 577: If it is
> nit: update a bit for better readability
Done
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/tablet/key_value_test_schema.h
File src/kudu/tablet/key_value_test_schema.h:
PS12, Line 24: #include <boost/optional/optional_io.hpp>
> nit: is it really needed after the change?
Done
PS12, Line 47: string ret = strings::Substitute("{$0,", key);
: if (val == boost::none) {
: ret.append("NULL}");
: } else {
: ret.append(strings::Substitute("$0}", *val));
: }
: return ret;
> nit: consider replacing with:
Done
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
PS12, Line 1754: tmp_snap_timestamp.FromUint64(scan_pb.snap_timestamp());
> nit: consider moving this after the if (!s.ok() && ...) check.
Done
PS12, Line 1757: !s.ok() && PREDICT_TRUE(!FLAGS_scanner_allow_snapshot_scans_with_logical_timestamps
> nit: does it make sense to check for the type of the error returned before
good point. Done
PS12, Line 1758: Snapshot scans not supported on this server
> nit: does it make sense to update the error message to give a hint on the n
I don't think so, honestly, this should only happen for LogicalClocks which are a test only thing
PS12, Line 1761: tmp_snap_timestamp > max_allowed_ts
> I might miss something here, but it seems it's always true if GetGlobalLate
if not set by GlobalLatest max_allowed_ts is kInvalidTimestmap, which in turn is max int64 -1
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4996
to look at the new patch set (#2).
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at timestamp.
When generating random operations scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save state at those (and only those)
timestamps while running.
This would fail very often before the REINSERT patches but
passes with it.
WIPish: This is just missing some dist-test runs but should
otherwise be ready for review.
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 199 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/2
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 10:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/4996/10//COMMIT_MSG
Commit Message:
PS10, Line 12: operations
> nit: comma after 'operations'
Done
Line 21: passes with it.
> did you loop it a bunch, particularly in an ASAN build?
yeah, unflaked fuzz-itest before this patch and then ran 500 loops of it. no failures.
http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
Line 80: using client::KuduInsert;
> warning: using decl 'KuduInsert' is unused [misc-unused-using-decls]
Done
PS10, Line 289: int timestamp,
: vector<ExpectedKeyValueRow> found,
: list<string>* errors
> can you doc these parameters? not obvious from the names what's going on
Done
PS10, Line 365: found
> std::move this?
Done
Line 372: CHECK(false) << final_error;
> LOG(FATAL)?
Done
PS10, Line 529: // Sort the scan timestamps in reverse order so that we can keep popping from the back and remove
: // duplicates.
: sort(timestamps_to_scan.begin(), timestamps_to_scan.end(), std::greater<int>());
: timestamps_to_scan.erase(unique(timestamps_to_scan.begin(),
: timestamps_to_scan.end()),
: timestamps_to_scan.end() );
> why not just use a std::set? this stuff isn't perf-critical so I think std:
we need to keep removing the smallest timestamp so we'd have to add complexity to scan the set anyway. mind if I leave it like this?
Line 576: sort(sorted_cur_val.begin(), sorted_cur_val.end());
> not quite sure what the sort is accomplishing here. Is it to move all of th
yeah, no need to sort, apparently. removed
http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/tablet/key_value_test_schema.h
File src/kudu/tablet/key_value_test_schema.h:
PS10, Line 46: bool operator < (const ExpectedKeyValueRow& other) const {
: return (key < other.key);
: }
:
: bool operator > (const ExpectedKeyValueRow& other) const {
: return (key > other.key);
: }
:
:
> a bit weird that these are not consistent with operator== above. I think yo
yeah, since there is no need to sort i removed these.
http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
Line 1760: } else if (tmp_snap_timestamp > max_allowed_ts) {
> warning: don't use else after return [readability-else-after-return]
Done
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4996
to look at the new patch set (#3).
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at timestamp.
When generating random operations scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save state at those (and only those)
timestamps while running.
This would fail very often before the REINSERT patches but
passes with it.
WIPish: This is just missing some dist-test runs but should
otherwise be ready for review.
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 197 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/3
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4996
to look at the new patch set (#10).
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at timestamp.
When generating random operations scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save state at those (and only those)
timestamps while running.
This would fail very often before the REINSERT patches but
passes with it.
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 191 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/10
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 292: found
nit: consider renaming it into something more tied to the scope of this method, e.g., 'rows' or 'rows_found'
PS12, Line 298: for (auto& found_row : found) {
: errors->push_back(Substitute("Found unexpected row: $0", found_row.ToString()));
: }
nit: there is a piece of syntactic sugar for this, if you wish:
transform(found.begin(), found.end(), back_inserter(*errors),
[](const ExpectedKeyValueRow& e){
return Substitute("unexpected row: $0", e.ToString());
});
PS12, Line 303: (*iter).
nit: using 'iter->first' instead could save 2 characters and easier to read, IMO :)
PS12, Line 304: vector<optional<ExpectedKeyValueRow>>
nit: could 'auto' work here as well?
PS12, Line 319:
nit: misaligned indent
PS12, Line 324:
nit: misaligned indent
PS12, Line 331:
nit: misaligned indent
PS12, Line 533: std::greater<int>()
> we pop from the back later, would prefer not to change to std::list
ah, I see -- I missed the pop part. Sure, that makes sense then.
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
nit: on the note of failing gracefully in case of unexpected things in the test environment, would it make sense to use just standard dynamic_cast<> and then call ASSERT_TRUE(result != nullptr) ?
PS12, Line 577: If it is
nit: update a bit for better readability
If it is, then store ...
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/tablet/key_value_test_schema.h
File src/kudu/tablet/key_value_test_schema.h:
PS12, Line 24: #include <boost/optional/optional_io.hpp>
nit: is it really needed after the change?
PS12, Line 47: string ret = strings::Substitute("{$0,", key);
: if (val == boost::none) {
: ret.append("NULL}");
: } else {
: ret.append(strings::Substitute("$0}", *val));
: }
: return ret;
nit: consider replacing with:
return strings::Substitute("{$0,$1}", key, val ? *val : "NULL");
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
PS12, Line 1754: tmp_snap_timestamp.FromUint64(scan_pb.snap_timestamp());
nit: consider moving this after the if (!s.ok() && ...) check.
PS12, Line 1757: !s.ok() && PREDICT_TRUE(!FLAGS_scanner_allow_snapshot_scans_with_logical_timestamps
nit: does it make sense to check for the type of the error returned before deducing the nature of the clock?
PS12, Line 1758: Snapshot scans not supported on this server
nit: does it make sense to update the error message to give a hint on the nature of the expected timestamp?
PS12, Line 1761: tmp_snap_timestamp > max_allowed_ts
I might miss something here, but it seems it's always true if GetGlobalLatest() call returns non-OK status. If so, in what regard the flag helps here?
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4996
to look at the new patch set (#13).
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at a timestamp.
When generating random operations, scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save the state immediately before those
(and only those) timestamps.
This would fail very often before the REINSERT patches but
passes with it.
Ran 500 loops of fuzz-itest in dist-test in ASAN with:
- KUDU_ALLOW_SLOW_TESTS=1
- --stress_cpu_threads=4
All tests passed. Result:
http://dist-test.cloudera.org//job?job_id=david.alves.1480096588.30319
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 200 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/13
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has uploaded a new patch set (#9).
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Add snapshot scans to fuzz-itest
This adds a new operation to fuzz-itest: snapshot scans
at timestamp.
When generating random operations scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save state at those (and only those)
timestamps while running.
This would fail very often before the REINSERT patches but
passes with it.
WIPish: This is just missing some dist-test runs but should
otherwise be ready for review.
Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 194 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/9
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add snapshot scans to fuzz-itest
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add snapshot scans to fuzz-itest
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
PS12, Line 574: down_cast<kudu::server::LogicalClock*>(
: tablet()->clock().get())
> well if you look at the down_cast definition in casts.h you'll see that in
How do you guarantee that all path which you code will see in release mode are the same which it sees in debug mode?
--
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes