[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hao Hao has posted comments on this change.
Change subject: KUDU-1875: Refuse unauthenticated connections from publicly
routable IP addrs
..
Patch Set 19:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/rpc/negotiation-test.cc
File src/kudu/rpc/negotiation-test.cc:
PS19, Line 155: 192.169
> since this is supposed to be testing a public IP, I think it's confusing th
Done
PS19, Line 220: unique_ptr server_socket = desc.use_test_socket ?
: unique_ptr(new
NegotiationTestSocket()) :
: unique_ptr(new
Socket());
> would be shorter to just write server_socket(desc.use_test_socket ? new Neg
Done
Line 923: }
> how about a case with an authenticated connection from a public routable IP
Done
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 61: DEFINE_bool(allow_unauthenticated_public_connections, false,
> Is this option actually still relevant if we have trusted_subnets? Isn't se
Done
PS19, Line 62: "Allow connections from unauthenticated public routable IPs. If
enabled, "
: "any people on the internet can connect and browse
the data, which is "
: "very dangerous. It is highly recommended to keep it
disabled.
> I'd suggest rephrasing this as:
Done
PS19, Line 157: encryption_ == RpcEncryption::DISABLED) {
> not quite sure whether this is right. I would have guessed that this would
Done
PS19, Line 879: unauthenticated
> this doesn't quite match the logic, if we're also checking encryption. ie t
Done
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS19, Line 500: vector addrs;
: RETURN_NOT_OK(GetNetworkAddresses(&addrs));
: string val = StrCat(FLAGS_trusted_subnets, JoinStrings(addrs,
","));
: SetCommandLineOptionWithMode("trusted_subnets",
:val.c_str(),
:google::SET_FLAGS_DEFAULT);
> it strikes me as a little strange to be writing back into the flags rather
Done
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/security/init.h
File src/kudu/security/init.h:
Line 34: Status InitTrustedSubnets();
> I think it would be better to encapsulate all of the logic somewhere in net
Done
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/net_util-test.cc
File src/kudu/util/net/net_util-test.cc:
Line 99: EXPECT_TRUE(addr.WithInSubnet("10.0.0.0/8"));
> in these tests it seems like you're always testing the case where the subne
It looks like "1.2.3.4/8" is legal address
https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing. Will add one test.
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:
PS19, Line 229: ==A
> nit: spaces around ==s
Done
Line 236: freeifaddrs(ifap);
> can you use MakeScopedCleanup here so that the freeifaddrs is placed as clo
Done
Line 333: // Strip any whitespace from the cidr_addr.
> should the stripping behavior be part of the ParseString below?
Done
Line 340: return (s.ok() && success);
> should verify that bits <= 32
Done
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
PS19, Line 110: // Returns the local network addresses.
: Status GetNetworkAddresses(std::vector* addresses);
> The docs should be clearer here to indicate that it's returning networks, n
Done
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/sockaddr.cc
File src/kudu/util/net/sockaddr.cc:
PS19, Line 118: std::pair p = strings::Split(net_cidr_addr,
:
strings::delimiter::Limit("/", 1));
:
: // Strip any whitespace from the cidr_addr.
: StripWhiteSpace(&p.first);
: Sockaddr net_addr;
: Status s = net_addr.ParseString(p.first, 0);
:
: uint32_t bits;
: bool success = SimpleAtoi(p.second, &bits);
> this seems to be duplicated code from elsewhere. Can you share this code?
Done
PS19, Line 130: NetworkByteOrder::FromHost32(~(0x >> bits))
> hm, I think I would have expected to see "(1ULL << bits) - 1"
Not sure if "(1ULL << bits) - 1" will work here, as net_mask should be big
endian?
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/sockaddr.h
File src/kudu/util/net/sockaddr.h:
PS19, Line 71: WithIn
> nit: "Within" rather than 'WithIn'
Done
http://gerrit.cloudera.org:8080/#/c/6514/19/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:
Line 100: virtual Status GetPeerAddress(Sockaddr *cur_ad
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6514 to look at the new patch set (#20). Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs .. KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs This rejects unauthenticated connections from publicly routable IPs, even if authentication and encryption are not configured. An adavance flag 'trusted_subnets', a trusted subnet whitelist. All unauthenticated or unencrypted connections are prohibited except these from the specified subnets and local subnets of all local network interfaces. Set the flag to '0.0.0.0/0' can completely disable this restriction. However network access is not otherwise restricted by a firewall, malicious users may be able to gain unauthorized access. Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e --- M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.h 9 files changed, 287 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/6514/20 -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 20 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Harsh J Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6514 to look at the new patch set (#21). Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs .. KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs This rejects unauthenticated connections from publicly routable IPs, even if authentication and encryption are not configured. An adavance flag 'trusted_subnets', a trusted subnet whitelist. All unauthenticated or unencrypted connections are prohibited except these from the specified subnets and local subnets of all local network interfaces. Set the flag to '0.0.0.0/0' can completely disable this restriction. However network access is not otherwise restricted by a firewall, malicious users may be able to gain unauthorized access. Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e --- M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.h 9 files changed, 288 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/6514/21 -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 21 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Harsh J Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6514 to look at the new patch set (#22). Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs .. KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs This rejects unauthenticated connections from publicly routable IPs, even if authentication and encryption are not configured. An adavance flag 'trusted_subnets', a trusted subnet whitelist. All unauthenticated or unencrypted connections are prohibited except these from the specified subnets and local subnets of all local network interfaces. Set the flag to '0.0.0.0/0' can completely disable this restriction. However network access is not otherwise restricted by a firewall, malicious users may be able to gain unauthorized access. Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e --- M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.h 9 files changed, 288 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/6514/22 -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 22 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Harsh J Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] log block manager: convert container metrics from counters to gauges
Todd Lipcon has posted comments on this change. Change subject: log block manager: convert container metrics from counters to gauges .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I343d99d42da7f4fbc0facb476c97f2ca8785031d Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-463. Add checksumming to cfile
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6630 to look at the new patch set (#17). Change subject: KUDU-463. Add checksumming to cfile .. KUDU-463. Add checksumming to cfile Adds optional checksumming and validation to cfile blocks. Introduces 2 flags to control cfile checksumming: - cfile_write_checksums (default false) - cfile_verify_checksums (default true) cfile_write_checksums is used in the CFileWriter to enable computing and appending Crc32 checksums to the end of each cfile block, header and footer cfile_write_checksums is defaulted to false to ensure upgrades don't immediately result in performance degredation and incompatible data on downgrade. It can and likely should be defaulted to true in a later release. When cfile_write_checksums is set to true, the existing incompatible_features bitset in the cfile footer is used. A "checksum" bit is set to ensure a clear error message when older versions of Kudu try to read the file. If checksums are not written the incompatible_features "checksum" bit is not set. cfile_verify_checksums is used in the CFileReader to enable validating the data against the written checksum. cfile_verify_checksums is defaulted to true since validation only occurs if checksums are written. Any data that was written before checksumming was an option or when cfile_write_checksums was false will not be verified. Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 --- M docs/design-docs/cfile.md M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_util.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/util/crc-test.cc M src/kudu/util/crc.cc M src/kudu/util/crc.h 10 files changed, 325 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/6630/17 -- To view, visit http://gerrit.cloudera.org:8080/6630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-463. Add checksumming to cfile
Grant Henke has posted comments on this change.
Change subject: KUDU-463. Add checksumming to cfile
..
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6630/15/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:
Line 146: if (PREDICT_FALSE(footer_->incompatible_features() &
~IncompatibleFeatures::SUPPORTED)) {
> eh, I still think that the "more correct" way is better. The point of using
Done
--
To view, visit http://gerrit.cloudera.org:8080/6630
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] log block manager: reopen container metadata writers at the OS level
Todd Lipcon has posted comments on this change.
Change subject: log block manager: reopen container metadata writers at the OS
level
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 2201: if (file_cache_) {
semi-unrelated question: now that we've had the file cache for a while and it
seems stable, can we start just assuming it's always on and reduce some of
these branches? Especially considering we don't test the "no cache" case, seems
like cruft we can remove
http://gerrit.cloudera.org:8080/#/c/6825/2/src/kudu/util/pb_util.h
File src/kudu/util/pb_util.h:
PS2, Line 280: Init
maybe best to rename this to OpenNew and the one below OpenForAppend or
OpenExisting? Despite the nice comments, I found the names a bit confusing.
--
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1549: compact LBM container metadata files at startup
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1549: compact LBM container metadata files at startup
..
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/fs_report.h
File src/kudu/fs/fs_report.h:
Line 127: Entry(std::string c, BlockRecordPB* r);
this is a little odd -- if it takes a pointer I expect it to store the pointer,
but the member is still a value. Add a comment? This is just a hack because
protobuf doesn't support move constructor?
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
PS2, Line 87: DEFINE_double(log_container_live_metadata_before_compact_ratio,
0.10,
: "Desired ratio of live block metadata in log
containers. If a "
: "container's live to total block ratio dips below
this value, "
: "the container's metadata file will be compacted at
startup.");
should probably be tagged advanced or even experimental?
0.1 seems like a pretty low value. Isn't the cost relatively low of writing out
a new file, considering they are small?
Line 417: BlockRecordPB* record,
comment on the pointer semantics. it's not clear what's being "saved" here
PS2, Line 1964: offsets also match
how could the offsets of two live blocks match?
PS2, Line 1969: .swap(records);
= std::move(records)? or use .emplace?
Line 2258: for (const auto& e : low_live_block_containers) {
can you extract the body of this if to a new function, perhaps? or too many
local variable references?
Line 2293: continue;
if this were extracted to a new function, this logic would be a bit easier to
follow (RETURN_NOT_OK is usable etc)
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
PS2, Line 295: // 3. Containers in 'low_live_block_containers' will have
their metadata
: //files compacted.
:
In a typical startup doesn't this mean a fairly large amount of memory usage,
vs doing it incrementally after loading each container? I see the appeal of
doing all mutative work in one pass at the end, but maybe it's not worth it
here, considering these aren't optional "repairs" of unexpected state, but
rather just a normal thing we expect would happen on basically every startup?
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/util/status.h
File src/kudu/util/status.h:
Line 60: #define KUDU_WARN_NOT_OK_AND_CONTINUE(to_call, warning_prefix) {
\
The naming here is a little confusing, considering 'continue' might be
interpreted as 'warn and keep going' rather than 'warn and use the continue
keyword to go to the top of the loop'. maybe CONTINUE_LOOP to clarify? or
perhaps extracting a functio where this is used could make it so we can use the
_AND_RETURN variant instead
--
To view, visit http://gerrit.cloudera.org:8080/6826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I981f7d9e7eb96fb40cef30ad96c5960b72d07756
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hao Hao has posted comments on this change. Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs .. Patch Set 22: TestNegotiation is passing locally. Checking why it is failed here. -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 22 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Harsh J Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Improve error message when trying to switch block managers
Hello Will Berkeley, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/6841 to review the following change. Change subject: Improve error message when trying to switch block managers .. Improve error message when trying to switch block managers Change-Id: I3ce8e03a07ff2e8fe074a77cc73d459e34354914 --- M src/kudu/fs/block_manager_util.cc 1 file changed, 4 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/6841/1 -- To view, visit http://gerrit.cloudera.org:8080/6841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3ce8e03a07ff2e8fe074a77cc73d459e34354914 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Improve error message when trying to switch block managers
Will Berkeley has posted comments on this change. Change subject: Improve error message when trying to switch block managers .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3ce8e03a07ff2e8fe074a77cc73d459e34354914 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Improve error message when trying to switch block managers
Will Berkeley has submitted this change and it was merged. Change subject: Improve error message when trying to switch block managers .. Improve error message when trying to switch block managers Change-Id: I3ce8e03a07ff2e8fe074a77cc73d459e34354914 Reviewed-on: http://gerrit.cloudera.org:8080/6841 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/fs/block_manager_util.cc 1 file changed, 4 insertions(+), 1 deletion(-) Approvals: Will Berkeley: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I3ce8e03a07ff2e8fe074a77cc73d459e34354914 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-463. Add checksumming to cfile
Todd Lipcon has posted comments on this change. Change subject: KUDU-463. Add checksumming to cfile .. Patch Set 17: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/6630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Will Berkeley has uploaded a new change for review. http://gerrit.cloudera.org:8080/6843 Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. KUDU-1056 and KUDU-1020 Safe time for ksck checksum Now that safe time works properly, this patch enables snapshot checksum scans in ksck. Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e --- M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_remote-test.cc 2 files changed, 18 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/6843/1 -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley
[kudu-CR] [c++ client] re-acquire authn token if expired
Todd Lipcon has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired .. Patch Set 12: Haven't followed the full discussion here, but it does seem like this is growing into a pretty large patch spanning KRPC and "kudu itself". Maybe worth taking a step back to write down and discuss the design here, and then we can split out the KRPC work from the specific work on the client? If I understand correctly, the overview is: - in order to fetch a new token, we need to make sure we're krb5-authenticated to the master(s) -- therefore, we need some way to make an RPC call ignore a pre-established token-authenticated connection and reconnect with krb5 -- this necessitates various changes in KRPC rather than just in our own client code Then it seems like there are a few possible alternatives how to do that -- either something in UserCredentials, something on the RpcController layer, etc, right? -- To view, visit http://gerrit.cloudera.org:8080/6648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Todd Lipcon has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/6843/1//COMMIT_MSG Commit Message: PS1, Line 9: Now that safe time works properly, this patch enables snapshot : checksum scans in ksck. the commit message doesn't seem quite right, since no changes to the actual ksck code are being made. This is just removing some workarounds from tests, no? http://gerrit.cloudera.org:8080/#/c/6843/1/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: Line 154: // Wait for the first 100 writes so each server should have a chance establish a timestamp. hm, not quite following this. shouldn't servers always have a timestamp? -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [c++ client] re-acquire authn token if expired
Alexey Serbin has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired .. Patch Set 12: > Haven't followed the full discussion here, but it does seem like > this is growing into a pretty large patch spanning KRPC and "kudu > itself". Maybe worth taking a step back to write down and discuss > the design here, and then we can split out the KRPC work from the > specific work on the client? > > If I understand correctly, the overview is: > - in order to fetch a new token, we need to make sure we're > krb5-authenticated to the master(s) > -- therefore, we need some way to make an RPC call ignore a > pre-established token-authenticated connection and reconnect with > krb5 > -- this necessitates various changes in KRPC rather than just in > our own client code > > Then it seems like there are a few possible alternatives how to do > that -- either something in UserCredentials, something on the > RpcController layer, etc, right? Right. At least I was looking at 3 different approaches only at KRPC to handle establishing and re-using a new connection to master servers which would be negotiated using non-token credentials. I think creating a small document to start discussion on the design is a way to do. I'll create a shared doc and send a link today. -- To view, visit http://gerrit.cloudera.org:8080/6648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [c++ client] re-acquire authn token if expired
Todd Lipcon has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired .. Patch Set 12: thanks. I don't think we need anything too formal in terms of a design doc, but just writing down a paragraph for each approach seems like a great place to start -- To view, visit http://gerrit.cloudera.org:8080/6648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6843 to look at the new patch set (#2). Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. KUDU-1056 and KUDU-1020 Safe time for ksck checksum Now that safe time works properly, this patch enables the snapshot checksum tests for ksck. Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e --- M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_remote-test.cc 2 files changed, 18 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/6843/2 -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Will Berkeley has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6843/1//COMMIT_MSG Commit Message: PS1, Line 9: Now that safe time works properly, this patch enables snapshot : checksum scans in ksck. > the commit message doesn't seem quite right, since no changes to the actual You're right. It just turns the tests on to make sure it's working. The code in ksck was already there. -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1875: Refuse unauthenticated connections from publicly
routable IP addrs
..
Patch Set 22:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/6514/22//COMMIT_MSG
Commit Message:
PS22, Line 13: adavance
typo: advanced
nit: change to a full sentence please
PS22, Line 17: However network
"However, if"
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 82: kudu::g_trusted_subnets->push_back(network);
I don't think the validator is supposed to have side effects -- it's not clear
how many times it will get called, etc
PS22, Line 85: can only be interpreted as
"must be specified in"
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:
Line 42: extern vector* g_trusted_subnets;
hm, where is this used externally? we try to avoid globals with such wide scope
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util-test.cc
File src/kudu/util/net/net_util-test.cc:
PS22, Line 95: TestPrivateAddresses
rename to TestWithinNetwork (this isn't specific to public/private)
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:
PS22, Line 186: string Network::ToString() const {
: return Substitute("$0:$1", addr_, mask_);
: }
this ToString isn't really human-readable. I think it would be better to either
implement one that returns something like CIDR or host/netmask, or just remove
it
Line 252: Status GetLocalNetwork(std::vector* net) {
nit: rename param to 'nets' since it is a list, not a single one
PS22, Line 256: auto cleanup = MakeScopedCleanup([&]() {
: freeifaddrs(ifap);
: });
this should probably be moved below the error check, otherwise we may
freeifaddrs() an uninitialized 'ifap'.
Alternatively, initialize 'ifap' to nullptr, and add an 'if (ifap)
freeifaddrs(ifap);' here
Line 265:
should probably 'net->clear()' here, since the docs say it gets them, not
appends them.
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
Line 98: Network(uint32_t addr, uint32_t mask);
add doc here regarding endianness, etc.
PS22, Line 107: ParseString
rename to ParseCIDR so it's clear this is a CIDR style string and not a
netmask-style
PS22, Line 111: mask_
I think conventionally people refer to this as 'netmask' instead of 'mask'. Can
you change here and the above parameters/getters/etc?
PS22, Line 131: GetLocalNetwork
since this gives back a list, change to GetLocalNetworks
--
To view, visit http://gerrit.cloudera.org:8080/6514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Harsh J
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-463. Add checksumming to cfile
Adar Dembo has posted comments on this change. Change subject: KUDU-463. Add checksumming to cfile .. Patch Set 17: Code-Review+2 Should I merge this? Or are you still doing performance testing? -- To view, visit http://gerrit.cloudera.org:8080/6630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hao Hao has posted comments on this change. Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs .. Patch Set 22: (1 comment) http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: Line 82: kudu::g_trusted_subnets->push_back(network); > I don't think the validator is supposed to have side effects -- it's not cl Hmm, what is the best way you think to init g_trusted_subnets? -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 22 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Harsh J Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-463. Add checksumming to cfile
Adar Dembo has submitted this change and it was merged. Change subject: KUDU-463. Add checksumming to cfile .. KUDU-463. Add checksumming to cfile Adds optional checksumming and validation to cfile blocks. Introduces 2 flags to control cfile checksumming: - cfile_write_checksums (default false) - cfile_verify_checksums (default true) cfile_write_checksums is used in the CFileWriter to enable computing and appending Crc32 checksums to the end of each cfile block, header and footer cfile_write_checksums is defaulted to false to ensure upgrades don't immediately result in performance degredation and incompatible data on downgrade. It can and likely should be defaulted to true in a later release. When cfile_write_checksums is set to true, the existing incompatible_features bitset in the cfile footer is used. A "checksum" bit is set to ensure a clear error message when older versions of Kudu try to read the file. If checksums are not written the incompatible_features "checksum" bit is not set. cfile_verify_checksums is used in the CFileReader to enable validating the data against the written checksum. cfile_verify_checksums is defaulted to true since validation only occurs if checksums are written. Any data that was written before checksumming was an option or when cfile_write_checksums was false will not be verified. Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 Reviewed-on: http://gerrit.cloudera.org:8080/6630 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon Reviewed-by: Adar Dembo --- M docs/design-docs/cfile.md M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_util.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/util/crc-test.cc M src/kudu/util/crc.cc M src/kudu/util/crc.h 10 files changed, 325 insertions(+), 56 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Todd Lipcon: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] fs manager: optimize tmp file deletion
Todd Lipcon has posted comments on this change.
Change subject: fs_manager: optimize tmp file deletion
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6837/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
PS1, Line 239: code:
: //
: // Status s = fs_manager_->Open();
: // if (s.IsNotFound()) {
: //
RETURN_NOT_OK(fs_manager_->CreateInitialFileSystemLayout());
: // s = fs_manager_->Open();
: // }
: // RETURN_NOT_OK(s);
I think instead of putting this code snippet here, you could just say "minimize
side effects in the case that the configured roots are not yet initialized"
--
To view, visit http://gerrit.cloudera.org:8080/6837
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1875: Refuse unauthenticated connections from publicly
routable IP addrs
..
Patch Set 22:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 82: kudu::g_trusted_subnets->push_back(network);
> Hmm, what is the best way you think to init g_trusted_subnets?
I think something like:
static std::once_flag once;
std::call_once(once, [*]() {
InitTrustedSubnets();
});
although keeping the validation there (without side effects) is also a good
idea.
--
To view, visit http://gerrit.cloudera.org:8080/6514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Harsh J
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1952 Remove round-robin for block placement
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6636 to look at the new patch set (#18). Change subject: KUDU-1952 Remove round-robin for block placement .. KUDU-1952 Remove round-robin for block placement This is the first of a multi-patch patchset to mitigate the single-disk failure. Throughout the code, the term "DataDir" refers to a data directory, which is often mounted on a distinct disk. Thus, "disks" and "data directories" will be used interchangeably. This patch adds a mapping from tablet to a set of disks and uses it to replace the existing round-robin placement of blocks. Tablets are mapped to a fixed number of disks (i.e. a DataDirGroup). New blocks are placed randomly in directories within each tablet's DataDirGroup. Tablet-to-group mappings are generated and stored as metadata upon tablet creation, or upon tablet replacement during a tablet copy. During group creation, disks are added to groups by randomly selecting two available directories and selecting the one with fewer tablets on it. This avoids pigeonholing new tablets to disks with relatively few tablets, while still trending towards filling underloaded disks. Groups are maintained when restarting the server, as they are flushed with metadata, and are deleted upon tablet deletion. When loading tablet data from a previous version of Kudu, the tablet's superblock will not have a DataDirGroup. One will be generated containing all data directories, as the tablet's data may already be spread across any number of disks. As this patch only addresses block placement, it does not itself mitigate single-disk failure. Given this, and given the tradeoff between I/O and failure disk-failure tolerance, the default behavior will be to spread tablet data across all disks. Testing is done at the block manager level in block_manager-test and log_block_manager-test, as well as in the new data_dirs-test. A design doc can be found here: https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit?usp=sharing Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b --- M src/kudu/cfile/bloomfile-test-base.h M src/kudu/cfile/cfile-test-base.h M src/kudu/cfile/cfile-test.cc M src/kudu/fs/CMakeLists.txt M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h A src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs.proto M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/delta_compaction-test.cc M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/delta_compaction.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/deltafile-test.cc M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 37 files changed, 986 insertions(+), 175 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/6636/18 -- To view, visit http://gerrit.cloudera.org:8080/6636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] WIP Don't suicide on EIO
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6773 to look at the new patch set (#4). Change subject: WIP Don't suicide on EIO .. WIP Don't suicide on EIO Rather than suiciding when reaching an EIO, this patch adds a mechanism that triggers error-handling in the form of a callback. This handler is attached to the lowest non-env operations that may result in EIO. E.g. PosixRWFile::Write() is an env operation that may result in an EIO. LogBlockContainer::WriteData()'s call to it must be wrapped in the new error-handling macro KUDU_RETURN_OR_HANDLE_EIO. Thus, all direct readers/writers of files must now implement EIO-handling code and wrap disk IO in KUDU_RETURN_OR_HANDLE_EIO. This patch is marked WIP, as the correct behavior after a disk fails is not yet included. For now, upon failure, the block managers will keep track of the dead disks and will mark the appropriate tablet peers as failed. Additionally, the error handling has been moved between layers offline, so more cleanup is being done to align with the above description. Change-Id: I4c221a5ea97c7b7ec5f8d395b5527dc16e63f828 --- M src/kudu/consensus/consensus_peers.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h A src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/sys_catalog.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet_peer_mm_ops.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/env.h M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h M src/kudu/util/status.h 24 files changed, 463 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/6773/4 -- To view, visit http://gerrit.cloudera.org:8080/6773 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4c221a5ea97c7b7ec5f8d395b5527dc16e63f828 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/6845 Change subject: WIP Allow external miniclusters to use many data dirs .. WIP Allow external miniclusters to use many data dirs In order to test different disk configurations, it is becoming increasingly important to have end-to-end testing with nodes backed by multiple directories. This patch adds this functionality to the ExternalMiniCluster class, which now supports the 'num_dirs_per_tserver' option, and the ExternalDaemon class, which now supports a list of data dirs instead of a single data dir. The ExternalMiniCluster will create the ExternalDaemon with a list of data dirs that reflects 'num_dirs_per_tserver' by using the original data dir name as a prefix and appending an integer (e.g. '/dir_name' with 'num_dirs_per_tserver = 2' will result in '/dir_name-0' and '/dir_name-1'). A new test called disk-failure-itest is added that exercises this. WIP: EIO-handling patch must be completed for further testing to make sense. Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332 --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/disk-failure-itest.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/integration-tests/ts_itest-base.h M src/kudu/util/path_util.cc M src/kudu/util/path_util.h 7 files changed, 266 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/6845/1 -- To view, visit http://gerrit.cloudera.org:8080/6845 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/6843/2/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: PS2, Line 154: a chance establish a timestamp. missing a "to" but in general what do you mean by "establish a timestamp"? -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Will Berkeley has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/6843/2/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: PS2, Line 154: a chance establish a timestamp. > missing a "to" but in general what do you mean by "establish a timestamp"? Without this change the test fails regularly with a message like: Service unavailable: Timed out: could not wait for desired snapshot timestamp to be consistent: Tablet is lagging too much to be able to serve snapshot scan. Lagging by: 1494362699845 ms, (max is 3 ms) from 1-2 tablets. Sometimes a minicluster replica hasn't ever updated its timestamp by the time the snapshot scan request reaches it. -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] categorization of rw operation failures
David Ribeiro Alves has posted comments on this change. Change subject: [catalog_manager] categorization of rw operation failures .. Patch Set 29: (6 comments) only nits, nearly there http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/integration-tests/catalog_manager_tsk-itest.cc File src/kudu/integration-tests/catalog_manager_tsk-itest.cc: PS29, Line 163: cluster_->Shutdown(); nit dont need this, the emc already shutsdown on the dctor http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS29, Line 500: might happen in case of a how about: can only happen in a PS29, Line 1026: PKI here and elsewhere, we tag our TODOs with usernames in most places PS29, Line 4126: "uninitialized" this is redundant http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: PS29, Line 360: // Calling this method makes sense only if leader_status.ok(). How about: Requires: leader_status() returns OK() http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: PS29, Line 202: that's a remove -- To view, visit http://gerrit.cloudera.org:8080/6170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Gerrit-PatchSet: 29 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [c++ client] re-acquire authn token if expired
Alexey Serbin has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired .. Patch Set 12: > thanks. I don't think we need anything too formal in terms of a > design doc, but just writing down a paragraph for each approach > seems like a great place to start I published a small write-up on this at https://docs.google.com/a/cloudera.com/document/d/1hjH9Y7yOJ6VQKPgpbKzjcUqhi7DyhcIgC3U8R_P6Ehs/edit?usp=sharing -- To view, visit http://gerrit.cloudera.org:8080/6648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1952 Remove round-robin for block placement
Andrew Wong has posted comments on this change.
Change subject: KUDU-1952 Remove round-robin for block placement
..
Patch Set 18:
(33 comments)
Failure from LINT and a memory leak in ASAN data_dirs-test dist-test (looking
into it).
http://gerrit.cloudera.org:8080/#/c/6636/17//COMMIT_MSG
Commit Message:
PS17, Line 9: mitigate the
: single-disk failure
> nit: we aren't mitigating the failure, but rather mitigating the effects of
Done
PS17, Line 27: When loading
: tablet data from a previous version of Kudu, the tablet's
superblock
: will not have a DataDirGroup. One will be generated containing
all
: data directories, as the tablet's data may already be spread
across
: any number of disks.
> what happens if we add a table on the new version, then downgrade, run for
Won't the superblock be rewritten with the old version of Kudu when new blocks
are written? I thought the superblock was always updated as new things get
flushed.
If that's the case, shouldn't the metadata be replaced with a group-less
version, and upon upgrading, we should be create a group with all dirs.
http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
Line 133: Status CountFilesCb(int* num_files, Env::FileType type,
> would using Env::Walk() make this easier?
Done
Line 428: size_t size1 = 5;
> Before asserting on the status' string, assert on its type (i.e. ASSERT_TRU
Done
http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
PS18, Line 154: t CountFiles_OLD(const string& path) {
: vector child_paths;
: Status s = env_->GetChildren(path, &child_paths);
: if (!s.ok()) {
: return 0;
: }
: int count = 0;
: for (const string& child_path : child_paths) {
: if (child_path == "." || child_path == ".." || child_path
== kInstanceMetadataFileName) {
: continue;
: }
: string full_child_path = JoinPathSegments(path,
child_path);
: bool is_dir;
: s = env_->IsDirectory(full_child_path, &is_dir);
: if (is_dir) {
: // Count the files in the child directories.
: count += CountFiles_OLD(full_child_path);
: } else {
: // Increment if the child is a file.
: count++;
: }
: }
: return count;
: }
Removed.
http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
PS17, Line 56: }
:
: pr
> You can omit this since it doesn't do anything beyond what the base class d
Done
PS17, Line 79: Status s = dd_manager_->GetNextDataDir(non_existent_opts,
nullptr);
: ASSERT_STR_CONTAIN
> Would it be possible to instantiate a DataDirManager and forgo the block ma
The main reason I added a blockmanager since setting up the env seemed to be
nicely handled.
Line 96: }
> Test the Status programmatically before testing its string representation.
Done
PS17, Line 115: TEST_F(DataDirGroupTest, TestDeleteDataDirGroup) {
: ASSERT_OK(dd_manager_->CreateDataDirGroup(test_
> Why is it necessary to set these? Won't GetNextDataDir() return Status::OK(
Left in from when I was experimenting with the flags. Removed
Line 138: // Add 20 tablets, each with size 3.
> Maybe do this once in the test constructor, and do it again in each test th
Done
http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS17, Line 62: riped ac
> maybe even experimental, considering this isn't quite usable yet?
Done
Line 188:
> ??
Good question. Removed.
PS17, Line 254: if (metric_entity) {
: metrics_.reset(new Data
> I don't think it makes sense to modify process-level state deep in an objec
RNG is used for creating groups and selecting directories within groups. I
added this here so my dist-test runs would differ between runs. Without it, the
groups would always be the same.
I've changed it to use GetRandomSeed32 and random_shuffle, which afaik don't
modify state.
Line 394: InsertOrDie(&uuid_idx_by_dd, dd.get(), idx);
> Don't need std:: prefixes here.
Done
PS17, Line 410:
> i think this and the one below could be a bit more specific and refer to da
Good call, done. Also removed "tracking" vernacular, since it's no longer used
in function names.
PS17, Line 422:
:
: // Adjust the disk group size to fit within the total number
of data dirs.
[kudu-CR] KUDU-1988: add support for advertised host:port info.
Patrik Sundberg has posted comments on this change.
Change subject: KUDU-1988: add support for advertised host:port info.
..
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/rpc_server.cc
File src/kudu/server/rpc_server.cc:
Line 121: }
> Instead of doing the empty check again below in GetAdvertisedAdddresses, yo
Done
Line 210: for (const Sockaddr addr : rpc_advertised_addresses_) {
> Make this a 'const Sockaddr&', otherwise it does an additional implicit cop
Done
http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/rpc_server.h
File src/kudu/server/rpc_server.h:
Line 70: Status GetAdvertisedAddresses(std::vector* addresses)
const WARN_UNUSED_RESULT;
> Have you tested this in your container setup? I'm worried that this interf
I have not checked yet, was meant to today but got sidelined. My thinking was
that as the advertised IP tends to by definition be a "public" IP, it tends to
always be resolvable by hosts on both internal and external networks. That's
the case in my test lab using docker swarm, and would be the case using
kubernetes on e.g. Google Cloud as well (my typical prod environment). I will
for sure check though!
http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
Line 208:80,
> Should this be opts.port?
possibly - I used how it's done for http_address_ up at line 120 as template,
figured perhaps was a reason why it's 80 there and not opts.port.Looking again
now I see http_address_ in the ctor is host:opts.port, and then it defaults to
port 80 later in BuildListenSpec. So probably, yes I'll change to opts.port.
Line 210: }
> Same comment here about checking emptiness upfront vs on each access.
understood, I'll work out best way to initialize.
--
To view, visit http://gerrit.cloudera.org:8080/6827
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Patrik Sundberg
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Patrik Sundberg
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1988: add support for advertised host:port info.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6827 to look at the new patch set (#5). Change subject: KUDU-1988: add support for advertised host:port info. .. KUDU-1988: add support for advertised host:port info. Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890 --- M src/kudu/master/master.cc M src/kudu/server/rpc_server.cc M src/kudu/server/rpc_server.h M src/kudu/server/webserver.cc M src/kudu/server/webserver.h M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/tserver/heartbeater.cc 8 files changed, 68 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/6827/5 -- To view, visit http://gerrit.cloudera.org:8080/6827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Patrik Sundberg Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Patrik Sundberg Gerrit-Reviewer: Tidy Bot
[kudu-CR] spark: add support for fault tolerant scanner
Jean-Daniel Cryans has posted comments on this change. Change subject: spark: add support for fault tolerant scanner .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/6782/6/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: Line 78: isFaultTolerant: Boolean, > This is a breaking change, so I think we should hold off on doing this. I' That's true, and putting it last with a default value isn't something we'd want to do? I guess because it doesn't scale to all the things we'd like to set? I wonder how other RDDs get around having many configurations. -- To view, visit http://gerrit.cloudera.org:8080/6782 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3f3192025ca5d74197600480fd3d040d70b4bbc2 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1988: add support for advertised host:port info.
Dan Burkert has posted comments on this change. Change subject: KUDU-1988: add support for advertised host:port info. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/rpc_server.h File src/kudu/server/rpc_server.h: Line 70: Status GetAdvertisedAddresses(std::vector* addresses) const WARN_UNUSED_RESULT; > I have not checked yet, was meant to today but got sidelined. My thinking w Oh interesting, if it's normal for the advertised address to be globally resolvable, then this way is fine. -- To view, visit http://gerrit.cloudera.org:8080/6827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Patrik Sundberg Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Patrik Sundberg Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1988: add support for advertised host:port info.
Patrik Sundberg has posted comments on this change. Change subject: KUDU-1988: add support for advertised host:port info. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/rpc_server.h File src/kudu/server/rpc_server.h: Line 70: Status GetAdvertisedAddresses(std::vector* addresses) const WARN_UNUSED_RESULT; > Oh interesting, if it's normal for the advertised address to be globally re I'm not aware of a case where it wouldn't be intended to be resolvable on both sides of the private/public split that's causing us to want to use an advertised address. And for cases when you want to use an advertised address, your deployment would be such that you can know at least the hostname which will get the public IP at config time, so hence that's what you use in config as the advertised name. At runtime this will be guaranteed to resolve, as it'd be done before container is running. -- To view, visit http://gerrit.cloudera.org:8080/6827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Patrik Sundberg Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Patrik Sundberg Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] [java-client] Update protoc and simplify the maven build
Grant Henke has uploaded a new change for review. http://gerrit.cloudera.org:8080/6846 Change subject: [java-client] Update protoc and simplify the maven build .. [java-client] Update protoc and simplify the maven build This patch simplifies the Java build by using maven to provide protoc via the Maven OS plugin and the protocArtifact property of the Maven Protobuf plugin. It also updates protoc to 3.3.0 and makes the required changes due to compatibility breaks. Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9 --- M docs/installation.adoc M java/README.md M java/kudu-client/pom.xml D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java M java/pom.xml 15 files changed, 60 insertions(+), 252 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/1 -- To view, visit http://gerrit.cloudera.org:8080/6846 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke
[kudu-CR] KUDU-1988: add support for advertised host:port info.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6827 to look at the new patch set (#6). Change subject: KUDU-1988: add support for advertised host:port info. .. KUDU-1988: add support for advertised host:port info. Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890 --- M src/kudu/master/master.cc M src/kudu/server/rpc_server.cc M src/kudu/server/rpc_server.h M src/kudu/server/webserver.cc M src/kudu/server/webserver.h M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/tserver/heartbeater.cc 8 files changed, 68 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/6827/6 -- To view, visit http://gerrit.cloudera.org:8080/6827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Patrik Sundberg Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Patrik Sundberg Gerrit-Reviewer: Tidy Bot
[kudu-CR] [java-client] Update protoc and simplify the maven build
Grant Henke has posted comments on this change. Change subject: [java-client] Update protoc and simplify the maven build .. Patch Set 1: Note: Currently compiling the proto files in this build will dump warnings about the syntax field missing. I will upgrade the proto version in the rest of the codebase and add syntax=proto2 in a follow up patch. -- To view, visit http://gerrit.cloudera.org:8080/6846 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] Update protoc and simplify the maven build
Dan Burkert has posted comments on this change. Change subject: [java-client] Update protoc and simplify the maven build .. Patch Set 1: (1 comment) I suspect the test failures may be due to endianess issues in the byte buffers. http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java: Line 30: import com.google.protobuf.UnsafeByteOperations; duplicate -- To view, visit http://gerrit.cloudera.org:8080/6846 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] Update protoc and simplify the maven build
Adar Dembo has posted comments on this change.
Change subject: [java-client] Update protoc and simplify the maven build
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/pom.xml
File java/kudu-client/pom.xml:
Line 144
To remove this, I think you need to add attachProtoSources=false to the
protobuf-maven-plugin configuration. Please test by extracting the kudu-client
JAR and looking for .proto files inside (there should be none).
Line 138:
com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}
If you don't mind please try this out on SLES 12 SP1, Red Hat or CentOS 6.6,
Ubuntu 14.04, and Debian 8. You don't actually need to run through the Java
build (unless you want to); IIUC there will be one executable for any x86_64
Linux, so just find it and try to run it on those platforms. If it fails to run
because of a missing glibc symbol version, we've got problems.
--
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] categorization of rw operation failures
Hello David Ribeiro Alves, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6170 to look at the new patch set (#30). Change subject: [catalog_manager] categorization of rw operation failures .. [catalog_manager] categorization of rw operation failures This changelist introduces the categorization of the system catalog's read and write operation failures which happen on leader post-election callback. There are two categories of errors: fatal and non-fatal. If an operation against the system catalog fails in between terms of the catalog leadership, the error is considered non-fatal. In case of a non-fatal error the leader post-election task bails out: the catalog is no longer the leader at the original term and the task should be executed by the new leader upon execution of the ElectedAsLeaderCb. If an operation against the system catalog fails within the same term of catalog leadership, the error is considered fatal and that causes the master process to crash (with an exception of writing a newly generated TSK when the TokenSigner still has a TSK to use). This is to avoid a possible inconsistency when working with the tables/tablets metadata, the IPKI certificate authority information and the Token Signing Keys. Any failure of a read or write operation against the system catalog happened during the catalog's shutdown is ignored and the leader post-election task bails out once detecting such failure. The same policy applies to other (i.e. not specific to read and write operations against the system catalog) errors which might happen while working with the IPKI certificate authority information and TokenSigner. The rationale is the same as for handling the system catalog operation failures: in case of an error, the leader has no consistent information to work with, meanwhile a non-leader does not use the information affected by the failure at all and can safely ignore the error. Added a test to verify that the master server does not crash if change of leadership detected while trying to persist a newly generated TSK (Token Signing Key). Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc 7 files changed, 506 insertions(+), 149 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6170/30 -- To view, visit http://gerrit.cloudera.org:8080/6170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Gerrit-PatchSet: 30 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [catalog manager] categorization of rw operation failures
Alexey Serbin has posted comments on this change. Change subject: [catalog_manager] categorization of rw operation failures .. Patch Set 29: (6 comments) yep, it's been nits for the last 3 revisions :) http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/integration-tests/catalog_manager_tsk-itest.cc File src/kudu/integration-tests/catalog_manager_tsk-itest.cc: PS29, Line 163: cluster_->Shutdown(); > nit dont need this, the emc already shutsdown on the dctor Done http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS29, Line 500: might happen in case of a > how about: can only happen in a Done PS29, Line 1026: PKI > here and elsewhere, we tag our TODOs with usernames in most places As I can see from the code, the better approach is to use TODO(KUDU-xxx) to have better tracking of issues. For this particular case, KUDU-1920 is opened. PS29, Line 4126: "uninitialized" > this is redundant Done http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: PS29, Line 360: // Calling this method makes sense only if leader_status.ok(). > How about: Requires: leader_status() returns OK() Done http://gerrit.cloudera.org:8080/#/c/6170/29/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: PS29, Line 202: that's a > remove Done -- To view, visit http://gerrit.cloudera.org:8080/6170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Gerrit-PatchSet: 29 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] benchmarks: make ensure cpu scaling more resilient
Hello Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/6847 to review the following change. Change subject: benchmarks: make ensure_cpu_scaling more resilient .. benchmarks: make ensure_cpu_scaling more resilient It's called out of a trap, and since the script uses pushd/popd a lot, it's not always reachable: + python write-jobs-stats-to-mysql.py kudu-benchmarks 1039 DenseNodeItest_time_bootstrapping_tablets 8 usage: write-jobs-stats-to-mysql.py + restore_governor + ensure_cpu_scaling performance ++ dirname ./src/kudu/scripts/benchmarks.sh + ./src/kudu/scripts/ensure_cpu_scaling.sh performance ./src/kudu/scripts/benchmarks.sh: line 93: ./src/kudu/scripts/ensure_cpu_scaling.sh: No such file or directory Change-Id: I3452b48577dd8f5f72c57d41ccf41848c682d616 --- M src/kudu/scripts/benchmarks.sh 1 file changed, 2 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/6847/1 -- To view, visit http://gerrit.cloudera.org:8080/6847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3452b48577dd8f5f72c57d41ccf41848c682d616 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Todd Lipcon
[kudu-CR] benchmarks: make ensure cpu scaling more resilient
Adar Dembo has posted comments on this change. Change subject: benchmarks: make ensure_cpu_scaling more resilient .. Patch Set 1: Verified+1 I tested this by running benchmarks.sh twice: once normally, and once I hit Ctrl-C _after_ a pushd. Both times ensure_cpu_scaling worked. -- To view, visit http://gerrit.cloudera.org:8080/6847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3452b48577dd8f5f72c57d41ccf41848c682d616 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] WIP Allow external miniclusters to use many data dirs
Adar Dembo has posted comments on this change. Change subject: WIP Allow external miniclusters to use many data dirs .. Patch Set 1: (1 comment) I'll hold off on a full review until you're more comfortable with this and the compile/test failures have been addressed, but I did glance at the API. http://gerrit.cloudera.org:8080/#/c/6845/1/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: Line 70: // Number of directories to create for each daemon. Wouldn't it be more natural to convert data_root into e.g. data_roots and allow multiple roots to be specified? That actually opens the door to running the mini cluster on a bunch of disks (provided the test code can collect the disk paths via gflag or some such). -- To view, visit http://gerrit.cloudera.org:8080/6845 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1952 Remove round-robin for block placement
Adar Dembo has posted comments on this change.
Change subject: KUDU-1952 Remove round-robin for block placement
..
Patch Set 18:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
Line 144:
Nit: got some extra whitespace here.
Line 325: CountFiles(path, &blocks_in_path);
Should ASSERT_OK() here.
http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
Line 96: }
> Done
Not done?
http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
PS18, Line 47: Env::Default()
Use env_
Line 72: testing::internal::Random r_;
This is a little unusual; use the Kudu Random from util/random.
http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 460: // natural tablet_id, select a data dir randomly.
> Done
Looks like you didn't add the DCHECK() though.
http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 21: #include
Nit: this belongs with the gflags/glog includes since boost is part of the
"project", not in the "system".
Line 540: Random r(GetRandomSeed32());
Let's make this a member of the DataDirManager and initialize it just once,
when constructing the DataDirManager.
Actually, let's make the output of std::default_random_engine() a member, and
seed it with GetRandomSeed32().
http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 51:
> No, forward declaration usually doesn't work for STL containers since I'm n
Right, I forgot about that. Sorry.
Could you put the DataDirGroup into a namespace called 'internal' to make it
clear that it's not for use outside of data_dirs.{h,cc}?
--
To view, visit http://gerrit.cloudera.org:8080/6636
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] categorization of rw operation failures
David Ribeiro Alves has posted comments on this change. Change subject: [catalog_manager] categorization of rw operation failures .. Patch Set 30: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Gerrit-PatchSet: 30 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [catalog manager] categorization of rw operation failures
David Ribeiro Alves has posted comments on this change. Change subject: [catalog_manager] categorization of rw operation failures .. Patch Set 30: there you go :) -- To view, visit http://gerrit.cloudera.org:8080/6170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Gerrit-PatchSet: 30 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [catalog manager] categorization of rw operation failures
David Ribeiro Alves has submitted this change and it was merged. Change subject: [catalog_manager] categorization of rw operation failures .. [catalog_manager] categorization of rw operation failures This changelist introduces the categorization of the system catalog's read and write operation failures which happen on leader post-election callback. There are two categories of errors: fatal and non-fatal. If an operation against the system catalog fails in between terms of the catalog leadership, the error is considered non-fatal. In case of a non-fatal error the leader post-election task bails out: the catalog is no longer the leader at the original term and the task should be executed by the new leader upon execution of the ElectedAsLeaderCb. If an operation against the system catalog fails within the same term of catalog leadership, the error is considered fatal and that causes the master process to crash (with an exception of writing a newly generated TSK when the TokenSigner still has a TSK to use). This is to avoid a possible inconsistency when working with the tables/tablets metadata, the IPKI certificate authority information and the Token Signing Keys. Any failure of a read or write operation against the system catalog happened during the catalog's shutdown is ignored and the leader post-election task bails out once detecting such failure. The same policy applies to other (i.e. not specific to read and write operations against the system catalog) errors which might happen while working with the IPKI certificate authority information and TokenSigner. The rationale is the same as for handling the system catalog operation failures: in case of an error, the leader has no consistent information to work with, meanwhile a non-leader does not use the information affected by the failure at all and can safely ignore the error. Added a test to verify that the master server does not crash if change of leadership detected while trying to persist a newly generated TSK (Token Signing Key). Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Reviewed-on: http://gerrit.cloudera.org:8080/6170 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc 7 files changed, 506 insertions(+), 149 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Gerrit-PatchSet: 31 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] log block manager: reopen container metadata writers at the OS level
Adar Dembo has posted comments on this change. Change subject: log block manager: reopen container metadata writers at the OS level .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 2201: w.reset(new WritablePBContainerFile(std::move(f))); > semi-unrelated question: now that we've had the file cache for a while and Sure, will do it in a follow-on patch. http://gerrit.cloudera.org:8080/#/c/6825/2/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: PS2, Line 280: Init > maybe best to rename this to OpenNew and the one below OpenForAppend or Ope OK, I'll use CreateNew() and OpenExisting(). -- To view, visit http://gerrit.cloudera.org:8080/6825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/6843/2/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: PS2, Line 154: a chance establish a timestamp. > Without this change the test fails regularly with a message like: I see what you mean, my comment was more about verbage. Are you sure that you might get that number from 2 replicas, not just 1? That lagging amount is interesting though, seems like the replica hasn't updated the safe time ever. Might actually be a bug. Don't want to delay this though, so regarding verbage how about: "Wait for the first 100 writes so be committed so that there is a very high chance that all replicas have committed at least one message in each tablet, otherwise safe time might not have been and replicas might refuse snapshot scans because of lag" os something like that -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] log block manager: reopen container metadata writers at the OS level
David Ribeiro Alves has posted comments on this change. Change subject: log block manager: reopen container metadata writers at the OS level .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: PS1, Line 649: DCHECK_EQ(FileState::NOT_INITIALIZED, state_); : RETURN_NOT_OK(ParsePBFileHeader(writer_.get(), &offset_, &version_)); : ContainerSupHeaderPB sup_header; : RETURN_NOT_OK(ReadSupplementalHeader(writer_.get(), version_, &offset_, &sup_header)); : RETURN_NOT_OK(writer_->Size(&offset_)); // Reset the write offset to the end of the file. : state_ = FileState::OPEN; > (I'm sure you're already aware of most of this long explanation, but I'm le nice writeup, could you copy paste this to the commit message? -- To view, visit http://gerrit.cloudera.org:8080/6825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] benchmarks: make ensure cpu scaling more resilient
David Ribeiro Alves has posted comments on this change. Change subject: benchmarks: make ensure_cpu_scaling more resilient .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3452b48577dd8f5f72c57d41ccf41848c682d616 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Will Berkeley has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/6843/2/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: PS2, Line 154: a chance establish a timestamp. > I see what you mean, my comment was more about verbage. Ah ok. Your suggestion is much clearer so I will use something like it. My comment about 1-2 tablets was misleading. More precisely, in the test table with 3 tablets, 9 replicas, I'd see 1-2 out of the 9 replicas have that problem-- not 2/3 replicas of the same tablet. -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6843 to look at the new patch set (#3). Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. KUDU-1056 and KUDU-1020 Safe time for ksck checksum Now that safe time works properly, this patch enables the snapshot checksum tests for ksck. Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e --- M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_remote-test.cc 2 files changed, 20 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/6843/3 -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1549: delete dead LBM containers at startup
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1549: delete dead LBM containers at startup .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I73a903092cf89508b7ba97fde3a2d6e691161b4c Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] benchmarks: make ensure cpu scaling more resilient
Adar Dembo has submitted this change and it was merged. Change subject: benchmarks: make ensure_cpu_scaling more resilient .. benchmarks: make ensure_cpu_scaling more resilient It's called out of a trap, and since the script uses pushd/popd a lot, it's not always reachable: + python write-jobs-stats-to-mysql.py kudu-benchmarks 1039 DenseNodeItest_time_bootstrapping_tablets 8 usage: write-jobs-stats-to-mysql.py + restore_governor + ensure_cpu_scaling performance ++ dirname ./src/kudu/scripts/benchmarks.sh + ./src/kudu/scripts/ensure_cpu_scaling.sh performance ./src/kudu/scripts/benchmarks.sh: line 93: ./src/kudu/scripts/ensure_cpu_scaling.sh: No such file or directory Change-Id: I3452b48577dd8f5f72c57d41ccf41848c682d616 Reviewed-on: http://gerrit.cloudera.org:8080/6847 Tested-by: Adar Dembo Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves --- M src/kudu/scripts/benchmarks.sh 1 file changed, 2 insertions(+), 3 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Adar Dembo: Verified Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I3452b48577dd8f5f72c57d41ccf41848c682d616 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] log block manager: reopen container metadata writers at the OS level
Adar Dembo has posted comments on this change. Change subject: log block manager: reopen container metadata writers at the OS level .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: PS1, Line 649: DCHECK_EQ(FileState::NOT_INITIALIZED, state_); : RETURN_NOT_OK(ParsePBFileHeader(writer_.get(), &offset_, &version_)); : ContainerSupHeaderPB sup_header; : RETURN_NOT_OK(ReadSupplementalHeader(writer_.get(), version_, &offset_, &sup_header)); : RETURN_NOT_OK(writer_->Size(&offset_)); // Reset the write offset to the end of the file. : state_ = FileState::OPEN; > nice writeup, could you copy paste this to the commit message? Sure why not. JD told me me people already expect walls of text from me anyway and I hate to disappoint. -- To view, visit http://gerrit.cloudera.org:8080/6825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Fix flaky test TestRecoverFromOpIdOverflow
David Ribeiro Alves has submitted this change and it was merged. Change subject: Fix flaky test TestRecoverFromOpIdOverflow .. Fix flaky test TestRecoverFromOpIdOverflow This test is flaky because we race against the COMMIT message for the first NO_OP in the WAL being written. It is currently hard to know when the actual COMMIT message is written to the WAL so we use a workaround to delete the first log segment before restarting the EMC in this test. If the COMMIT doesn't get written in time then the tablet bootstrap process doesn't prune that entry at startup time and it ends up in the new log prior to the much higher-numbered log entries. The flaky test failure was due to an out of order log index being detected, and looked like the following error in the log: F0504 21:28:21.128690 13908 raft_consensus_state.cc:502] Check failed: _s.ok() Bad status: Corruption: New operation's index does not follow the previous op's index. Current: 2147483648.2147483648. Previous: 1.1 *** Check failure stack trace: *** @ 0x7fe0adef915d google::LogMessage::Fail() at ??:0 @ 0x7fe0adefb05d google::LogMessage::SendToLog() at ??:0 @ 0x7fe0adef8c99 google::LogMessage::Flush() at ??:0 @ 0x7fe0adefbaff google::LogMessageFatal::~LogMessageFatal() at ??:0 @ 0x7fe0b57a8018 kudu::consensus::PendingRounds::AdvanceCommittedIndex() at ??:0 @ 0x7fe0b57848a5 kudu::consensus::RaftConsensus::NotifyCommitIndex() at ??:0 @ 0x7fe0b5749ccf kudu::consensus::PeerMessageQueue::NotifyObserversOfCommitIndexChangeTask() at ??:0 @ 0x7fe0b575bdc1 kudu::internal::RunnableAdapter<>::Run() at ??:0 Change-Id: Ib382819307da04bb76d68d2c015dc0edd9f60267 Reviewed-on: http://gerrit.cloudera.org:8080/6808 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves --- M src/kudu/integration-tests/ts_recovery-itest.cc 1 file changed, 28 insertions(+), 0 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib382819307da04bb76d68d2c015dc0edd9f60267 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] log block manager: reopen container metadata writers at the OS level
David Ribeiro Alves has posted comments on this change. Change subject: log block manager: reopen container metadata writers at the OS level .. Patch Set 2: gotta meet expectations -- To view, visit http://gerrit.cloudera.org:8080/6825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] log block manager: reopen container metadata writers at the OS level
David Ribeiro Alves has posted comments on this change. Change subject: log block manager: reopen container metadata writers at the OS level .. Patch Set 2: (and as long as you're writing the walls of text anyway might as well put them front and center, for posterity) -- To view, visit http://gerrit.cloudera.org:8080/6825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2005: actionable error messages from webserver
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/6848 Change subject: KUDU-2005: actionable error messages from webserver .. KUDU-2005: actionable error messages from webserver As it turned out, squeasel outputs only errors via the log_message callback. Let's output them with ERROR severity into Kudu log (they were output as INFO messages prior to this patch). Also, if the embedded squeasel webserver fails to start, add the last error message from it (if any) into the RuntimeError status message returned from Webserver::Start(). Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77 --- M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver.h 3 files changed, 25 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6848/1 -- To view, visit http://gerrit.cloudera.org:8080/6848 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] log: release WritableLogSegment buffers when log is idle
David Ribeiro Alves has posted comments on this change. Change subject: log: release WritableLogSegment buffers when log is idle .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/6611/3/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: PS3, Line 199: 1 flag? http://gerrit.cloudera.org:8080/#/c/6611/3/src/kudu/consensus/log.h File src/kudu/consensus/log.h: Line 90: extra line PS3, Line 99: Status SwitchToAsyncMode(); this new state change seems weird because it's one way. could we just refactor Append to just be wrapper around Status AsyncAppend(std::unique_ptr entry_batch, const StatusCallback& callback) and always have the log in async mode? do you think there would be a perf impact? -- To view, visit http://gerrit.cloudera.org:8080/6611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I512c7f9264ac9490e6a57259e4d7b2608adc1ca7 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2005: actionable error messages from webserver
Will Berkeley has posted comments on this change. Change subject: KUDU-2005: actionable error messages from webserver .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6848/1/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS1, Line 246: TryRunLsof(addr); Put this after the error message below, since the below puts it in context. It'd also be nice to do this lsof step only when we get an address-in-use error, or have good reason to suspect that is the error, rather than for any failure. -- To view, visit http://gerrit.cloudera.org:8080/6848 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1549: compact LBM container metadata files at startup
Adar Dembo has posted comments on this change.
Change subject: KUDU-1549: compact LBM container metadata files at startup
..
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/fs_report.h
File src/kudu/fs/fs_report.h:
Line 127: Entry(std::string c, BlockRecordPB* r);
> this is a little odd -- if it takes a pointer I expect it to store the poin
I'm pining away for protobuf move constructors.
(will add comment).
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
PS2, Line 87: DEFINE_double(log_container_live_metadata_before_compact_ratio,
0.10,
: "Desired ratio of live block metadata in log
containers. If a "
: "container's live to total block ratio dips below
this value, "
: "the container's metadata file will be compacted at
startup.");
> should probably be tagged advanced or even experimental?
I'll tag it advanced. I don't think experimental is appropriate given that
modifying it doesn't cause new code paths to execute; it just adjusts how often
they run.
As for the default value...I don't have a strong opinion, but I'll note that
the higher it is, the more metadata write amplification we suffer. That's why I
was conservative to start. Did you have a particular value in mind?
Line 417: BlockRecordPB* record,
> comment on the pointer semantics. it's not clear what's being "saved" here
Done
PS2, Line 1964: offsets also match
> how could the offsets of two live blocks match?
It's contrived, but the two blocks could be of zero length. I'll add a comment.
PS2, Line 1969: .swap(records);
> = std::move(records)? or use .emplace?
Done
Line 2258: for (const auto& e : low_live_block_containers) {
> can you extract the body of this if to a new function, perhaps? or too many
Done
Line 2293: continue;
> if this were extracted to a new function, this logic would be a bit easier
Agreed, done.
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
PS2, Line 295: // 3. Containers in 'low_live_block_containers' will have
their metadata
: //files compacted.
:
> In a typical startup doesn't this mean a fairly large amount of memory usag
I went back and forth on this. I do like the idea of not making any on-disk
changes until we've established that there are no fatal errors, and that's why
I ended up on this side of the fence.
That said, here's an alternative: if you'll allow me to grow LogBlock by 8
bytes, I can add the block creation timestamp to it, and then I can use the
LogBlocks that we've already allocated in the compaction. There's another
advantage to this approach: it opens the door to doing compaction at real time
without the onerous burden of rereading records from disk.
Why didn't I do this already? I couldn't bring myself to grow our memory
footprint by potentially dozens of MB without also implementing real time
metadata compaction.
http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/util/status.h
File src/kudu/util/status.h:
Line 60: #define KUDU_WARN_NOT_OK_AND_CONTINUE(to_call, warning_prefix) {
\
> The naming here is a little confusing, considering 'continue' might be inte
I extracted a function like you suggested, so this has been removed.
--
To view, visit http://gerrit.cloudera.org:8080/6826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I981f7d9e7eb96fb40cef30ad96c5960b72d07756
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] fs manager: optimize tmp file deletion
Adar Dembo has posted comments on this change.
Change subject: fs_manager: optimize tmp file deletion
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6837/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
PS1, Line 239: code:
: //
: // Status s = fs_manager_->Open();
: // if (s.IsNotFound()) {
: //
RETURN_NOT_OK(fs_manager_->CreateInitialFileSystemLayout());
: // s = fs_manager_->Open();
: // }
: // RETURN_NOT_OK(s);
> I think instead of putting this code snippet here, you could just say "mini
Done
--
To view, visit http://gerrit.cloudera.org:8080/6837
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07830f0cc3fd5da847361607c62a369c39e677d2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1549: delete dead LBM containers at startup
Adar Dembo has submitted this change and it was merged. Change subject: KUDU-1549: delete dead LBM containers at startup .. KUDU-1549: delete dead LBM containers at startup Full containers with no live blocks are "dead" in that they will never be used for any block operations. These containers are safe to delete, and though we lose some forensic information (e.g. block X was created at time Y and deleted at time Z) in doing so, we'll wind up processing less container metadata in the next startup. This patch adds a quick and dirty implementation of dead container deletion at startup. It would be nice to also do it in real time (perhaps as a maintenance manager operation), but that requires containers to have shared ownership, a lifecycle change I'm not keen on making right now. Change-Id: I73a903092cf89508b7ba97fde3a2d6e691161b4c Reviewed-on: http://gerrit.cloudera.org:8080/6824 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves --- M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h 3 files changed, 157 insertions(+), 20 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I73a903092cf89508b7ba97fde3a2d6e691161b4c Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] log block manager: convert container metrics from counters to gauges
Adar Dembo has submitted this change and it was merged. Change subject: log block manager: convert container metrics from counters to gauges .. log block manager: convert container metrics from counters to gauges Container deletion is just around the corner, so these metrics need to become gauges so that they can be decremented. As an aside, it's possible that we've already been mishandling these counters: they should never drop in value, even across restarts, and we were incrementing them every time a container was reopened at startup. However, we may have been OK because the block manager loads before the web UI, so by the time the metrics are fetchable, the counter values should be the same as what they were pre-restart. Change-Id: I343d99d42da7f4fbc0facb476c97f2ca8785031d Reviewed-on: http://gerrit.cloudera.org:8080/6823 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 2 files changed, 18 insertions(+), 26 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I343d99d42da7f4fbc0facb476c97f2ca8785031d Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2005: actionable error messages from webserver
Alexey Serbin has posted comments on this change. Change subject: KUDU-2005: actionable error messages from webserver .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6848/1/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS1, Line 246: TryRunLsof(addr); > Put this after the error message below, since the below puts it in context. I'm not sure I understand -- the error message below is generated from squeasel error message and the http_address_. TryRunLsof() just outputs its messages into the log, since the second argument of TryRunLsof() is empty. Also, that output goes into the WARNING log file, not ERROR one. I looked into the squeasel source code and I could not find the way to detect that condition if not parsing the error message returned from it. That's the excerpt: cry(fc(ctx), "%s: cannot bind to %.*s: %d (%s)", __func__, (int) vec.len, vec.ptr, ERRNO, strerror(errno)); So, I would rather leave it as is for now since I'm not a fan of parsing error messages. Expecting/checking for particular error message patterns in tests is more or less tolerable, but not here. There are too many risks: the squeasel code can change, that particular error message might be not the last one we receive, etc. Let's change the output from TryRunLsof() to remove the ' "Failed to bind to " << addr.ToString() << ". "' part -- that would be less confusion to the readers (I will post that patch separately). -- To view, visit http://gerrit.cloudera.org:8080/6848 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-2001 Add UNDO size to tablet on-disk size
Will Berkeley has uploaded a new change for review. http://gerrit.cloudera.org:8080/6850 Change subject: KUDU-2001 Add UNDO size to tablet on-disk size .. KUDU-2001 Add UNDO size to tablet on-disk size To make the on-disk size metric more accurate, this patch makes DeltaTracker::EstimateOnDiskSize() count UNDO deltas and adds a new method DeltaTracker::EstimateOnDiskSizeForCompaction() that retains the original behavior, which only counted REDOs and was used for compaction scoring. Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/tablet.cc 5 files changed, 25 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/6850/1 -- To view, visit http://gerrit.cloudera.org:8080/6850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley
[kudu-CR] KUDU-2005: actionable error messages from webserver
Will Berkeley has posted comments on this change. Change subject: KUDU-2005: actionable error messages from webserver .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/6848/1/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS1, Line 246: TryRunLsof(addr); > I'm not sure I understand -- the error message below is generated from sque Thanks for looking into the squeasel code to see if we could do better re: lsof check. I agree it's best not to parse the message. -- To view, visit http://gerrit.cloudera.org:8080/6848 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-2001 Add UNDO size to tablet on-disk size
Will Berkeley has uploaded a new patch set (#2). Change subject: KUDU-2001 Add UNDO size to tablet on-disk size .. KUDU-2001 Add UNDO size to tablet on-disk size To make the on-disk size metric more accurate, this patch makes DeltaTracker::EstimateOnDiskSize() count UNDO deltas and adds a new method DeltaTracker::EstimateOnDiskSizeForCompaction() that retains the original behavior, which only counts REDOs and is used for compaction scoring. Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/tablet.cc 5 files changed, 25 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/6850/2 -- To view, visit http://gerrit.cloudera.org:8080/6850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1056 and KUDU-1020 Safe time for ksck checksum
Will Berkeley has posted comments on this change. Change subject: KUDU-1056 and KUDU-1020 Safe time for ksck checksum .. Patch Set 3: Verified+1 -raft_consensus-itest failure is a flake I've seen a lot on master -MultiThreadedTabletTest appears to be a rare known failure that is unrelated to the patch -- To view, visit http://gerrit.cloudera.org:8080/6843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib45be20dcfa37fb85185302adf84d2c4a55f8c1e Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] KUDU-2001 Add UNDO size to tablet on-disk size
Will Berkeley has posted comments on this change. Change subject: KUDU-2001 Add UNDO size to tablet on-disk size .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6850/1/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: Line 151: using base::subtle::Barrier_AtomicIncrement; > warning: using decl 'Barrier_AtomicIncrement' is unused [misc-unused-using- Done -- To view, visit http://gerrit.cloudera.org:8080/6850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1941: more validation for RPC auth flags
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/6851 Change subject: KUDU-1941: more validation for RPC auth flags .. KUDU-1941: more validation for RPC auth flags With this patch, both master and tserver refuse to start if authentication is 'required' but no authentication method is configured. Prior to this patch, the inconsistency with the run-time configuration could be detected at a later stage when a client would try to connect to Kudu cluster. Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51 --- M src/kudu/rpc/messenger.cc 1 file changed, 11 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/6851/1 -- To view, visit http://gerrit.cloudera.org:8080/6851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] KUDU-2005: actionable error messages from webserver
Alexey Serbin has submitted this change and it was merged. Change subject: KUDU-2005: actionable error messages from webserver .. KUDU-2005: actionable error messages from webserver As it turned out, squeasel outputs only errors via the log_message callback. Let's output them with ERROR severity into Kudu log (they were output as INFO messages prior to this patch). Also, if the embedded squeasel webserver fails to start, add the last error message from it (if any) into the RuntimeError status message returned from Webserver::Start(). Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77 Reviewed-on: http://gerrit.cloudera.org:8080/6848 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver.h 3 files changed, 25 insertions(+), 6 deletions(-) Approvals: Will Berkeley: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6848 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1941: more validation for RPC auth flags
Adar Dembo has posted comments on this change. Change subject: KUDU-1941: more validation for RPC auth flags .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1941: more validation for RPC auth flags
Alexey Serbin has submitted this change and it was merged. Change subject: KUDU-1941: more validation for RPC auth flags .. KUDU-1941: more validation for RPC auth flags With this patch, both master and tserver refuse to start if authentication is 'required' but no authentication method is configured. Prior to this patch, the inconsistency with the run-time configuration could be detected at a later stage when a client would try to connect to Kudu cluster. Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51 Reviewed-on: http://gerrit.cloudera.org:8080/6851 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/rpc/messenger.cc 1 file changed, 11 insertions(+), 2 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I3c088fd6d7a695234e2955e09ca53626078b4e51 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] updated output from TryRunLsof()
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/6852 Change subject: [util] updated output from TryRunLsof() .. [util] updated output from TryRunLsof() TryRunLsof() should not conclude it's called because of an error binding to the specified address: in some use-cases it might not be the case. The output is updated to remove the statement about a failure to bind to the specified address, leaving just the output from lsof. This is a pure cosmetic fix to improve error message readability in the context of KUDU-2005. Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 --- M src/kudu/util/net/net_util.cc 1 file changed, 3 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6852/1 -- To view, visit http://gerrit.cloudera.org:8080/6852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] [util] updated output from TryRunLsof()
Adar Dembo has posted comments on this change. Change subject: [util] updated output from TryRunLsof() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6852/1/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: PS1, Line 293: if Nit: it was more grammatically correct without 'if'. -- To view, visit http://gerrit.cloudera.org:8080/6852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] [util] updated output from TryRunLsof()
Alexey Serbin has uploaded a new patch set (#2). Change subject: [util] updated output from TryRunLsof() .. [util] updated output from TryRunLsof() TryRunLsof() should not conclude it's called because of an error binding to the specified address: in some use-cases it might not be the case. The output is updated to remove the statement about a failure to bind to the specified address, leaving just the output from lsof. This is a pure cosmetic fix to improve error message readability in the context of KUDU-2005. Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 --- M src/kudu/util/net/net_util.cc 1 file changed, 3 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6852/2 -- To view, visit http://gerrit.cloudera.org:8080/6852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [util] updated output from TryRunLsof()
Alexey Serbin has posted comments on this change. Change subject: [util] updated output from TryRunLsof() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6852/1/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: PS1, Line 293: if > Nit: it was more grammatically correct without 'if'. Done -- To view, visit http://gerrit.cloudera.org:8080/6852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] [util] updated output from TryRunLsof()
Adar Dembo has posted comments on this change. Change subject: [util] updated output from TryRunLsof() .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] KUDU-1192 Periodically flush glog buffers from a thread
William Li has uploaded a new change for review. http://gerrit.cloudera.org:8080/6853 Change subject: KUDU-1192 Periodically flush glog buffers from a thread .. KUDU-1192 Periodically flush glog buffers from a thread Added a timer inside the AsyncLogger to periodically calls the flush(). It wakes up the main RunThread() to flush the messages from buffers Period to wake up is default to 1 second but can also use environment variable to change it Change-Id: Id4c6d440e9259efcf222530f13137f7de5bf00fc --- M src/kudu/util/async_logger.cc M src/kudu/util/async_logger.h M src/kudu/util/logging-test.cc 3 files changed, 113 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/6853/1 -- To view, visit http://gerrit.cloudera.org:8080/6853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Id4c6d440e9259efcf222530f13137f7de5bf00fc Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: William Li
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Mike Percy has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/consensus/metadata.proto File src/kudu/consensus/metadata.proto: PS6, Line 103: There is a corner case in Raft where a node may be elected leader of a : // pending (uncommitted) configuration. In such a case, the master will : // detect that the node is not a member of the committed configuration. I think you can just say: The leader may be a part of the committed or the pending configuration (or both). What the master does with that information isn't part of the contract of this data structure. PS6, Line 113: 5 4 http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS6, Line 195: nit: stray space here PS6, Line 199: nit: funny extra indentation Line 219: string DiffConsensusStates(const ConsensusStatePB& old_state, Looks like this needs to detect differences in pending configs now too. http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS6, Line 2235: nit: indent at least an extra 4 spaces for a line wrap PS6, Line 2362: Previously, this field was omitted from the : // consensus state in this situation. I don't think this helps clarify this code, consider removing this part of the comment. http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: Line 134: RETURN_NOT_OK(consensus::VerifyConsensusState(cstate)); Add something like CHECK(!cstate.has_pending_config()) since we should never persist a pending config. http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/tserver/tablet_copy.proto File src/kudu/tserver/tablet_copy.proto: PS6, Line 116: committed remove this word http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS6, Line 951: *reported_tablet->mutable_consensus_state() = : consensus->ConsensusState(); nit: This will fit on one line now -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-2001 Add UNDO size to tablet on-disk size
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6850 to look at the new patch set (#3). Change subject: KUDU-2001 Add UNDO size to tablet on-disk size .. KUDU-2001 Add UNDO size to tablet on-disk size To make the on-disk size metric more accurate, this patch makes DeltaTracker::EstimateOnDiskSize() count UNDO deltas and adds a new method DeltaTracker::EstimateOnDiskSizeForCompaction() that retains the original behavior, which only counts REDOs and is used for compaction scoring. Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/tablet.cc 5 files changed, 25 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/6850/3 -- To view, visit http://gerrit.cloudera.org:8080/6850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2001 Add UNDO size to tablet on-disk size
Will Berkeley has posted comments on this change. Change subject: KUDU-2001 Add UNDO size to tablet on-disk size .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/6850/2/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: Line 152: using kudu::consensus::OpId; > warning: using decl 'OpId' is unused [misc-unused-using-decls] Done Line 153: using kudu::consensus::MaximumOpId; > warning: using decl 'MaximumOpId' is unused [misc-unused-using-decls] Done -- To view, visit http://gerrit.cloudera.org:8080/6850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59001adadb9a768a464e7b2cf0f0a5df0ef5393a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] log: release WritableLogSegment buffers when log is idle
Adar Dembo has posted comments on this change. Change subject: log: release WritableLogSegment buffers when log is idle .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/6611/3/src/kudu/consensus/log.h File src/kudu/consensus/log.h: PS3, Line 99: Status SwitchToAsyncMode(); > this new state change seems weird because it's one way. could we just refac My issue with an explicit "async mode" is that it exposes the append thread's existence (or non-existence) outside this class. To reduce our overall thread count, an "idle" append thread should exit and a future async operation should start it up again, but that becomes harder to do if the append thread's existence spills out into other classes via this "async mode" thing. I guess if there were a "switch back to sync mode" function then it'd be OK, but ideally this class would manage the starting/stopping of the append thread automatically, without forcing its users to do it. Incidentally, did you explore having the append thread exit outright on idle, and bringing it back up on the next async operation? The synchronization gets hairy... -- To view, visit http://gerrit.cloudera.org:8080/6611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I512c7f9264ac9490e6a57259e4d7b2608adc1ca7 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util] updated output from TryRunLsof()
Alexey Serbin has submitted this change and it was merged. Change subject: [util] updated output from TryRunLsof() .. [util] updated output from TryRunLsof() TryRunLsof() should not conclude it's called because of an error binding to the specified address: in some use-cases it might not be the case. The output is updated to remove the statement about a failure to bind to the specified address, leaving just the output from lsof. This is a pure cosmetic fix to improve error message readability in the context of KUDU-2005. Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 Reviewed-on: http://gerrit.cloudera.org:8080/6852 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/util/net/net_util.cc 1 file changed, 3 insertions(+), 4 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1ff0d28f631c7f392420040b132f7542f08b74d6 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [java-client] Update protoc and simplify the maven build
Grant Henke has posted comments on this change.
Change subject: [java-client] Update protoc and simplify the maven build
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/pom.xml
File java/kudu-client/pom.xml:
Line 144
> To remove this, I think you need to add attachProtoSources=false to the pro
I think this step was just trying to remove the test proto's due to some
unusual behavior. See "**/*test*.proto".
I tested and no test proto is included after my change. Both before and after
this change 25 kudu proto files are included in the final jar. I also verified
with a jar in maven central.
Line 138:
com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}
> If you don't mind please try this out on SLES 12 SP1, Red Hat or CentOS 6.6
Will test and report back.
http://gerrit.cloudera.org:8080/#/c/6846/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java:
Line 30: import com.google.protobuf.UnsafeByteOperations;
> duplicate
Done
--
To view, visit http://gerrit.cloudera.org:8080/6846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java-client] Update protoc and simplify the maven build
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6846 to look at the new patch set (#2). Change subject: [java-client] Update protoc and simplify the maven build .. [java-client] Update protoc and simplify the maven build This patch simplifies the Java build by using maven to provide protoc via the Maven OS plugin and the protocArtifact property of the Maven Protobuf plugin. It also updates protoc to 3.3.0 and makes the required changes due to compatibility breaks. Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9 --- M docs/installation.adoc M java/README.md M java/kudu-client/pom.xml D java/kudu-client/src/main/java/com/google/protobuf/ZeroCopyLiteralByteString.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java M java/pom.xml 15 files changed, 64 insertions(+), 252 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/6846/2 -- To view, visit http://gerrit.cloudera.org:8080/6846 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I70f7ad260777d5355497fa7e9a1047c342ff9ee9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6514 to look at the new patch set (#23). Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs .. KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs This rejects unauthenticated connections from publicly routable IPs, even if authentication and encryption are not configured. An adavanced flag 'trusted_subnets' is provided to whitelist trusted subnets. All unauthenticated or unencrypted connections are prohibited except these from the specified subnets and local subnets of all local network interfaces. Set the flag to '0.0.0.0/0' can completely disable this restriction. However network access is not otherwise restricted by a firewall, malicious users may be able to gain unauthorized access. Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e --- M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.h 9 files changed, 317 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/6514/23 -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 23 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Harsh J Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs
Hao Hao has posted comments on this change.
Change subject: KUDU-1875: Refuse unauthenticated connections from publicly
routable IP addrs
..
Patch Set 22:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/6514/22//COMMIT_MSG
Commit Message:
PS22, Line 13: adavance
> typo: advanced
Done
PS22, Line 17: However network
> "However, if"
Done
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 82: kudu::g_trusted_subnets->push_back(network);
> I think something like:
Done
PS22, Line 85: can only be interpreted as
> "must be specified in"
Done
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:
Line 42: extern vector* g_trusted_subnets;
> hm, where is this used externally? we try to avoid globals with such wide s
Done
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util-test.cc
File src/kudu/util/net/net_util-test.cc:
PS22, Line 95: TestPrivateAddresses
> rename to TestWithinNetwork (this isn't specific to public/private)
Done
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:
PS22, Line 186: string Network::ToString() const {
: return Substitute("$0:$1", addr_, mask_);
: }
> this ToString isn't really human-readable. I think it would be better to ei
Done
Line 252: Status GetLocalNetwork(std::vector* net) {
> nit: rename param to 'nets' since it is a list, not a single one
Done
PS22, Line 256: auto cleanup = MakeScopedCleanup([&]() {
: freeifaddrs(ifap);
: });
> this should probably be moved below the error check, otherwise we may freei
Done
Line 265:
> should probably 'net->clear()' here, since the docs say it gets them, not a
Done
http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
Line 98: Network(uint32_t addr, uint32_t mask);
> add doc here regarding endianness, etc.
Done
PS22, Line 107: ParseString
> rename to ParseCIDR so it's clear this is a CIDR style string and not a net
Done
PS22, Line 111: mask_
> I think conventionally people refer to this as 'netmask' instead of 'mask'.
Done
PS22, Line 131: GetLocalNetwork
> since this gives back a list, change to GetLocalNetworks
Done
--
To view, visit http://gerrit.cloudera.org:8080/6514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Harsh J
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1192 Periodically flush glog buffers from a thread
Todd Lipcon has posted comments on this change. Change subject: KUDU-1192 Periodically flush glog buffers from a thread .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/6853/1/src/kudu/util/async_logger.cc File src/kudu/util/async_logger.cc: Line 34: const char* kAsyncLoggerTimerEnvVar = "KUDU_ASYNC_LOGGER_TIMER"; rather than using an env var, should use a gflag for the period at which this is flushed. We can probably just reuse the existing FLAGS_logbufsecs in fact Line 54: timer_thread_ = std::thread(&AsyncLogger::RunTimer, this); instead of a separate thread, why not just change the existing thread so that instead of doing a Wait() with no timeout, it does a Wait() with a timeout of logbufsecs and then wakes up and flushes even if nothing has been enqueued? Line 148: wake_flusher_cond_.Wait(); eg here you can use a TimedWait, and if it times out, set active_buf_->flush = true -- To view, visit http://gerrit.cloudera.org:8080/6853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id4c6d440e9259efcf222530f13137f7de5bf00fc Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: William Li Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
