You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/08/13 03:11:36 UTC

[kudu-CR] monotime: remove granularity argument

Hello Dan Burkert, Mike Percy, Adar Dembo, Alexey Serbin,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: monotime: remove granularity argument
......................................................................

monotime: remove granularity argument

In practice, we almost never used the 'COARSE' granularity.
Instead, it just added an extra crufty argument to hundreds of call
sites.

Let's remove it for now so that the API is a little simpler. If we ever
have a perf-sensitive use case where COARSE makes sense, we can revive
this.

monotime.h is part of the public-facing client API, but MonoTime isn't
used in any public-facing methods. So, I think it's safe to remove the
argument without considering this a breaking change.

Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
---
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/consensus_queue.h
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/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-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/tablet_copy-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/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/result_tracker.cc
M src/kudu/rpc/rpc-stress-test.cc
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/server/hybrid_clock-test.cc
M src/kudu/server/hybrid_clock.cc
M src/kudu/server/logical_clock-test.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.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/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
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-test.cc
M src/kudu/util/failure_detector.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
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/threadpool.cc
M src/kudu/util/throttler-test.cc
88 files changed, 293 insertions(+), 320 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] monotime: remove granularity argument

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has submitted this change and it was merged.

Change subject: monotime: remove granularity argument
......................................................................


monotime: remove granularity argument

In practice, we almost never used the 'COARSE' granularity.
Instead, it just added an extra crufty argument to hundreds of call
sites.

Let's remove it for now so that the API is a little simpler. If we ever
have a perf-sensitive use case where COARSE makes sense, we can revive
this.

monotime.h is part of the public-facing client API, but MonoTime isn't
used in any public-facing methods. So, I think it's safe to remove the
argument without considering this a breaking change.

Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
Reviewed-on: http://gerrit.cloudera.org:8080/3966
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
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/consensus_queue.h
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/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-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/tablet_copy-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/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/result_tracker.cc
M src/kudu/rpc/rpc-stress-test.cc
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/server/hybrid_clock-test.cc
M src/kudu/server/hybrid_clock.cc
M src/kudu/server/logical_clock-test.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.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/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
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-test.cc
M src/kudu/util/failure_detector.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
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/threadpool.cc
M src/kudu/util/throttler-test.cc
88 files changed, 293 insertions(+), 320 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Alexey Serbin: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] monotime: remove granularity argument

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

Kudu Jenkins has posted comments on this change.

Change subject: monotime: remove granularity argument
......................................................................


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2859/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No

[kudu-CR] monotime: remove granularity argument

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: monotime: remove granularity argument
......................................................................


Patch Set 1: Code-Review+2

Personally, I'm fine with the outright removal.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No

[kudu-CR] monotime: remove granularity argument

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

Alexey Serbin has posted comments on this change.

Change subject: monotime: remove granularity argument
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

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

Line 13: Let's remove it for now so that the API is a little simpler. If we ever
Having this clean-up is great!

As an alternative, consider using FINE as parameter-by-default for MonoTime::Now().

Or it contradicts with our current C++ code guideline?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] monotime: remove granularity argument

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: monotime: remove granularity argument
......................................................................


Patch Set 1:

(1 comment)

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

Line 13: Let's remove it for now so that the API is a little simpler. If we ever
> Having this clean-up is great!
Yea, I considered just making it a default parameter, but it's "discouraged" in the guidelines (though we use them sometimes). I couldn't really find any good reasons for COARSE to exist, though -- it seems like the only time we care about performance of MonoTime::Now() is when we're timing a very fast-running function and don't want the time-collection overhead to drown out the actual function runtime. But, in that case, we also need high precision for the time to be correct, so COARSE isnt that useful anyway.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1d375d54e8598105f3ec833c37d96a382c1a8f7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes