[kudu-CR] [util/monotime] added handy operators for MonoTime
Todd Lipcon has submitted this change and it was merged. Change subject: [util/monotime] added handy operators for MonoTime .. [util/monotime] added handy operators for MonoTime Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Reviewed-on: http://gerrit.cloudera.org:8080/3999 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/client/batcher.cc M src/kudu/client/client-internal.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/meta_cache.cc M src/kudu/client/scan_token-internal.cc M src/kudu/client/scanner-internal.cc M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log_anchor_registry.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/delete_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/test_workload.cc M src/kudu/integration-tests/write_throttling-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master_rpc.cc M src/kudu/master/ts_descriptor.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/reactor-test.cc M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc.cc M src/kudu/rpc/rpc_controller.cc M src/kudu/rpc/rpc_stub-test.cc M src/kudu/rpc/sasl_rpc-test.cc M src/kudu/rpc/service_queue.h M src/kudu/server/hybrid_clock-test.cc M src/kudu/server/hybrid_clock.cc M src/kudu/server/pprof-path-handlers.cc M src/kudu/server/server_base.cc M src/kudu/server/webui_util.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/tablet_mm_ops-test.cc M src/kudu/tablet/tablet_peer.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/tablet/transactions/transaction_tracker.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_remote-test.cc M src/kudu/tserver/heartbeater.cc M src/kudu/tserver/scanner_metrics.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_server-stress-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc M src/kudu/util/countdown_latch.h M src/kudu/util/debug/trace_event_synthetic_delay.cc M src/kudu/util/failure_detector.cc M src/kudu/util/metrics.cc M src/kudu/util/monotime-test.cc M src/kudu/util/monotime.cc M src/kudu/util/monotime.h M src/kudu/util/net/socket.cc M src/kudu/util/striped64-test.cc M src/kudu/util/striped64.cc M src/kudu/util/test_util.cc M src/kudu/util/threadpool.cc M src/kudu/util/throttler-test.cc M src/kudu/util/throttler.cc M src/kudu/util/trace-test.cc 84 files changed, 736 insertions(+), 370 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 6 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/monotime] added handy operators for MonoTime
Alexey Serbin has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 5: (1 comment) Thank you for the review! http://gerrit.cloudera.org:8080/#/c/3999/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS5, Line 2455: deadline_ = start_ts_ + MonoDelta::FromMilliseconds(FLAGS_tablet_creation_timeout_ms); > Can this be moved into the initializer list? The deadline_ is a member of the base-base class, so it's not possible here. -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 5 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 Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Adar Dembo has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/3999/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS5, Line 2455: deadline_ = start_ts_ + MonoDelta::FromMilliseconds(FLAGS_tablet_creation_timeout_ms); Can this be moved into the initializer list? -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 5 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 Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3999 to look at the new patch set (#5). Change subject: [util/monotime] added handy operators for MonoTime .. [util/monotime] added handy operators for MonoTime Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db --- M src/kudu/client/batcher.cc M src/kudu/client/client-internal.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/meta_cache.cc M src/kudu/client/scan_token-internal.cc M src/kudu/client/scanner-internal.cc M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log_anchor_registry.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/delete_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/test_workload.cc M src/kudu/integration-tests/write_throttling-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master_rpc.cc M src/kudu/master/ts_descriptor.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/reactor-test.cc M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc.cc M src/kudu/rpc/rpc_controller.cc M src/kudu/rpc/rpc_stub-test.cc M src/kudu/rpc/sasl_rpc-test.cc M src/kudu/rpc/service_queue.h M src/kudu/server/hybrid_clock-test.cc M src/kudu/server/hybrid_clock.cc M src/kudu/server/pprof-path-handlers.cc M src/kudu/server/server_base.cc M src/kudu/server/webui_util.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/tablet_mm_ops-test.cc M src/kudu/tablet/tablet_peer.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/tablet/transactions/transaction_tracker.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_remote-test.cc M src/kudu/tserver/heartbeater.cc M src/kudu/tserver/scanner_metrics.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_server-stress-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc M src/kudu/util/countdown_latch.h M src/kudu/util/debug/trace_event_synthetic_delay.cc M src/kudu/util/failure_detector.cc M src/kudu/util/metrics.cc M src/kudu/util/monotime-test.cc M src/kudu/util/monotime.cc M src/kudu/util/monotime.h M src/kudu/util/net/socket.cc M src/kudu/util/striped64-test.cc M src/kudu/util/striped64.cc M src/kudu/util/test_util.cc M src/kudu/util/threadpool.cc M src/kudu/util/throttler-test.cc M src/kudu/util/throttler.cc M src/kudu/util/trace-test.cc 84 files changed, 736 insertions(+), 370 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/3999/5 -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 5 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/monotime] added handy operators for MonoTime
Kudu Jenkins has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/2963/ -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 5 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 Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Kudu Jenkins has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2960/ -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 4 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 Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3999 to look at the new patch set (#4). Change subject: [util/monotime] added handy operators for MonoTime .. [util/monotime] added handy operators for MonoTime Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db --- M src/kudu/client/batcher.cc M src/kudu/client/client-internal.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/meta_cache.cc M src/kudu/client/scan_token-internal.cc M src/kudu/client/scanner-internal.cc M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/delete_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/test_workload.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc M src/kudu/master/master_rpc.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc.cc M src/kudu/rpc/rpc_stub-test.cc M src/kudu/rpc/sasl_rpc-test.cc M src/kudu/rpc/service_queue.h M src/kudu/server/hybrid_clock-test.cc M src/kudu/server/pprof-path-handlers.cc M src/kudu/server/server_base.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/tablet_mm_ops-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_remote-test.cc M src/kudu/tserver/heartbeater.cc M src/kudu/tserver/tablet_service.cc M src/kudu/util/debug/trace_event_synthetic_delay.cc M src/kudu/util/metrics.cc M src/kudu/util/monotime-test.cc M src/kudu/util/monotime.cc M src/kudu/util/monotime.h M src/kudu/util/test_util.cc M src/kudu/util/throttler-test.cc M src/kudu/util/throttler.cc M src/kudu/util/trace-test.cc 53 files changed, 501 insertions(+), 275 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/3999/4 -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 4 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/monotime] added handy operators for MonoTime
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3999 to look at the new patch set (#3). Change subject: [util/monotime] added handy operators for MonoTime .. [util/monotime] added handy operators for MonoTime Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db --- M src/kudu/client/batcher.cc M src/kudu/client/client-internal.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/meta_cache.cc M src/kudu/client/scan_token-internal.cc M src/kudu/client/scanner-internal.cc M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/delete_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/test_workload.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc M src/kudu/master/master_rpc.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc.cc M src/kudu/rpc/rpc_stub-test.cc M src/kudu/rpc/sasl_rpc-test.cc M src/kudu/rpc/service_queue.h M src/kudu/server/hybrid_clock-test.cc M src/kudu/server/pprof-path-handlers.cc M src/kudu/server/server_base.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/tablet_mm_ops-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck_remote-test.cc M src/kudu/tserver/heartbeater.cc M src/kudu/tserver/tablet_service.cc M src/kudu/util/debug/trace_event_synthetic_delay.cc M src/kudu/util/metrics.cc M src/kudu/util/monotime-test.cc M src/kudu/util/monotime.cc M src/kudu/util/monotime.h M src/kudu/util/test_util.cc M src/kudu/util/throttler-test.cc M src/kudu/util/throttler.cc M src/kudu/util/trace-test.cc 53 files changed, 499 insertions(+), 271 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/3999/3 -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 3 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/monotime] added handy operators for MonoTime
Kudu Jenkins has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2958/ -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 3 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 Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Alexey Serbin has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/3999/2/src/kudu/util/monotime.h File src/kudu/util/monotime.h: Line 246: /// The other object to compare with. > I forgot, there's this bit from the Google style guide: Yep, that's a good idea. Will do. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Adar Dembo has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.h File src/kudu/util/monotime.h: Line 201: /// than the specified one. > Good catch! I'll remove this modification since it could break backward-co Right, without the operators I understand the motivation, but with them it doesn't seem as necessary. http://gerrit.cloudera.org:8080/#/c/3999/2/src/kudu/util/monotime.h File src/kudu/util/monotime.h: Line 246: bool operator==(const MonoTime& other) const; I forgot, there's this bit from the Google style guide: "Prefer to define non-modifying binary operators as non-member functions. If a binary operator is defined as a class member, implicit conversions will apply to the right-hand argument, but not the left-hand one. It will confuse your users if a < b compiles but b < a doesn't." Looks like ==, !=, <, <=, >, and <= fit that classification; could you redefine them as non-member functions? -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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 Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Kudu Jenkins has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2956/ -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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 Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3999 to look at the new patch set (#2). Change subject: [util/monotime] added handy operators for MonoTime .. [util/monotime] added handy operators for MonoTime Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db --- M src/kudu/util/monotime-test.cc M src/kudu/util/monotime.cc M src/kudu/util/monotime.h 3 files changed, 311 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/3999/2 -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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/monotime] added handy operators for MonoTime
Alexey Serbin has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: > (1 comment) Great! That's simplifies everything. Then I'll remove those obsolete checks. -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Alexey Serbin has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.cc File src/kudu/util/monotime.cc: Line 209: // A sanity check: nanos_ is unsigned > The second check is to ensure that abs(delta.nano_delta) <= nanos_ if delta typo, there should be 'replace (-delta.nano_delta <= nanos_) with (delta.nano_delta < 0 && abs(delta.nano_delta) <= nanos)' Line 271: DCHECK(ts.tv_nsec >= 0 || -ts.tv_nsec <= nanos_); > I'll copy-paste the explanation here: typo, there should be 'replace (-delta.nano_delta <= nanos_) with (delta.nano_delta < 0 && abs(delta.nano_delta) <= nanos)' -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Todd Lipcon has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.cc File src/kudu/util/monotime.cc: Line 210: DCHECK(delta.nano_delta_ >= 0 || -delta.nano_delta_ <= nanos_); > Yep, will do -- thanks. I explained my motivation for not using it in one I recently switched nanos_ to be a signed integer so that negative monotimes are actually allowed (was necessary for some code in ResultTracker GC to work on systems that had recently booted and thus had low MonoTimes) -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Alexey Serbin has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: > I was going to say that the style guide frowns upon overloading, > but apparently it was updated in September 2015 to now encourage it > instead of methods like 'CopyFrom' and 'Equals'. Either way, I > think this is a good use case -- I'm certainly tired of writing the > old: > > MonoTime deadline = MonoTime::Now(); > deadline.AddDelta(MonoDelta::FromSeconds(10)); > > vs a more natural '+' here > > Didn't look at the code, but consider me +1 on the idea. Let's also > try to sweep for call sites and update them if it's not too much > hassle. Yep, with those operators you can write MonoTime deadline = MonoTime::Now() + MonoDelta::FromSeconds(10); -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Alexey Serbin has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: > I was going to say that the style guide frowns upon overloading, > but apparently it was updated in September 2015 to now encourage it > instead of methods like 'CopyFrom' and 'Equals'. Either way, I > think this is a good use case -- I'm certainly tired of writing the > old: > > MonoTime deadline = MonoTime::Now(); > deadline.AddDelta(MonoDelta::FromSeconds(10)); > > vs a more natural '+' here > > Didn't look at the code, but consider me +1 on the idea. Let's also > try to sweep for call sites and update them if it's not too much > hassle. OK, thanks - I will look at different places where it's called to replace those. This patch is a part of my changes for AUTO_BACKGROUND_FLUSH work: Dan proposed to separate it into a standalone change. Since it's a separate entity now, I think it's OK to update those places. -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db 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: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Alexey Serbin has posted comments on this change.
Change subject: [util/monotime] added handy operators for MonoTime
..
Patch Set 1:
(12 comments)
Will post a new version shortly.
http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime-test.cc
File src/kudu/util/monotime-test.cc:
Line 211: MonoTime m_end = start.AddDelta(MonoDelta(-delta.nano_delta_));
> Use ToNanoseconds() to avoid test friendship?
OK, if you think it's a good idea, I'll do. My original thought was that it's
internal implementation detail, and as I understand there is no guarantee that
nanoseconds is the actual granularity of MonoDelta. Using the private
constructor that works with internals looked more compelling to me.
http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.cc
File src/kudu/util/monotime.cc:
Line 209: // A sanity check: nanos_ is unsigned
> I don't understand the second check. If 'delta' is negative, why must its p
The second check is to ensure that abs(delta.nano_delta) <= nanos_ if
delta.nano_delta is negative to prevent things like uint64_t x = 100UL - 200UL.
If it makes things clearer, I can replace (-delta.nano_delta <= nanos_) with
(delta.nano_delta < 0 || abs(delta.nano_delta) <= nanos)
Line 210: DCHECK(delta.nano_delta_ >= 0 || -delta.nano_delta_ <= nanos_);
> Can't you call delta.ToNanoseconds() to extract the nanos, instead of using
Yep, will do -- thanks. I explained my motivation for not using it in one of
the earlier comments.
Line 235: this->AddDelta(MonoDelta(-1 * delta.nano_delta_));
> Can use delta.ToNanoseconds() here.
Done
Line 244: return !(*this == other);
> How about !this->Equals(other)? Seems easier to compare with operator==.
Yep, I think it's subjective -- to me it looked otherwise :) Will replaceq.
Line 271: DCHECK(ts.tv_nsec >= 0 || -ts.tv_nsec <= nanos_);
> Again, I don't understand the second half of this check.
I'll copy-paste the explanation here:
The second check is to ensure that abs(delta.nano_delta) <= nanos_ if
delta.nano_delta is negative to prevent things like uint64_t x = 100UL - 200UL.
If it makes things clearer, I can replace (-delta.nano_delta <= nanos_) with
(delta.nano_delta < 0 || abs(delta.nano_delta) <= nanos)
Line 299: return MonoTime(t).AddDelta(MonoDelta(-delta.nano_delta_));
> Can use ToNanoseconds() here.
Done
http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.h
File src/kudu/util/monotime.h:
Line 201: MonoTime& AddDelta(const MonoDelta &delta);
> Hmm, is this ABI compatible with the old AddDelta()? It may not matter; the
Good catch! I'll remove this modification since it could break
backward-compatibility.
As for the motivation, look at the MonoTime operator +(const MonoTime& t, const
MonoDelta& delta;
Also, I wanted to write one-line code like
MonoTime& MonoTime::operator +=(const MonoDelta& delta) {
return this->AddDelta(delta);
}
Another point was before I added those operators: it came from the code which
looked like the following:
const MonoDelta delta(MonoDelta::FromMilliseconds(MAX_WAIT_MS));
const MonoTime tmp(MonoTime::Now());
const MonoTime deadline_0(tmp.AddDelta(delta));
const MonoTime deadline_1(tmp.AddDelta(delta));
Line 232: MonoTime& operator +=(const MonoDelta& delta);
> Can you use "operator" instead of "operator " (note the lack of
Done
Line 251: /// Check if the object represents a different point in time.
> Nit: "...than the given one."
Done
Line 321: MonoDelta KUDU_EXPORT operator -(const MonoTime& t0, const MonoTime&
t1);
> The Google style guide frowns on operator overloading when it's incomplete
The motivation for this was the code like this:
MonoDelta t_diff = t_end - t_begin;
EXPECT_GT(wait_timeout_ms / 10, t_diff.ToMilliseconds());
This is because I was confused by MonoTime::GetDeltaSince(): should I write
t_end.GetDeltaSince(t_begin) or t_begin.GetDeltaSince(t_end) to get the desired
result? Using the operator clears those questions once and for all.
Line 323: /// Add the specified interval to the given point in time.
> Nit: "time interval".
Done
--
To view, visit http://gerrit.cloudera.org:8080/3999
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db
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: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Todd Lipcon has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: I was going to say that the style guide frowns upon overloading, but apparently it was updated in September 2015 to now encourage it instead of methods like 'CopyFrom' and 'Equals'. Either way, I think this is a good use case -- I'm certainly tired of writing the old: MonoTime deadline = MonoTime::Now(); deadline.AddDelta(MonoDelta::FromSeconds(10)); vs a more natural '+' here Didn't look at the code, but consider me +1 on the idea. Let's also try to sweep for call sites and update them if it's not too much hassle. -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Adar Dembo has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: (12 comments) Is the use case for these guys very compelling? Although the Google style guide permits operator overloading (subject to some conditions), we're generally pretty judicious with our use of it. I'm not opposed to this patch, but I think we should have a pretty compelling set of use cases to merit merging it. http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime-test.cc File src/kudu/util/monotime-test.cc: Line 211: MonoTime m_end = start.AddDelta(MonoDelta(-delta.nano_delta_)); Use ToNanoseconds() to avoid test friendship? http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.cc File src/kudu/util/monotime.cc: Line 209: // A sanity check: nanos_ is unsigned I don't understand the second check. If 'delta' is negative, why must its positive version be less than or equal to nanos_? Line 210: DCHECK(delta.nano_delta_ >= 0 || -delta.nano_delta_ <= nanos_); Can't you call delta.ToNanoseconds() to extract the nanos, instead of using friendship? Line 235: this->AddDelta(MonoDelta(-1 * delta.nano_delta_)); Can use delta.ToNanoseconds() here. Line 244: return !(*this == other); How about !this->Equals(other)? Seems easier to compare with operator==. Line 271: DCHECK(ts.tv_nsec >= 0 || -ts.tv_nsec <= nanos_); Again, I don't understand the second half of this check. Line 299: return MonoTime(t).AddDelta(MonoDelta(-delta.nano_delta_)); Can use ToNanoseconds() here. http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.h File src/kudu/util/monotime.h: Line 201: MonoTime& AddDelta(const MonoDelta &delta); Hmm, is this ABI compatible with the old AddDelta()? It may not matter; the client API never uses MonoTimes, just MonoDeltas. But I'm still curious. What's the motivation here, anyway? Support fluent-style AddDelta() chaining? Is that really something we need? Line 232: MonoTime& operator +=(const MonoDelta& delta); Can you use "operator" instead of "operator " (note the lack of space"? I think that'd be more consistent with overloaded operators elsewhere in the codebase. Line 251: /// Check if the object represents a different point in time. Nit: "...than the given one." Line 321: MonoDelta KUDU_EXPORT operator -(const MonoTime& t0, const MonoTime& t1); The Google style guide frowns on operator overloading when it's incomplete (in this case, overloading operator- without also overloading operator+). Of course, operator+ for two MonoTimes doesn't make any sense, so perhaps we should drop this overload? Line 323: /// Add the specified interval to the given point in time. Nit: "time interval". -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [util/monotime] added handy operators for MonoTime
Kudu Jenkins has posted comments on this change. Change subject: [util/monotime] added handy operators for MonoTime .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2939/ -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [util/monotime] added handy operators for MonoTime
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/3999 Change subject: [util/monotime] added handy operators for MonoTime .. [util/monotime] added handy operators for MonoTime Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db --- M src/kudu/util/monotime-test.cc M src/kudu/util/monotime.cc M src/kudu/util/monotime.h 3 files changed, 312 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/3999/1 -- To view, visit http://gerrit.cloudera.org:8080/3999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
