You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2024/05/09 05:33:20 UTC
[kudu-CR] [common] get rid of MutexLock
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21415
Change subject: [common] get rid of MutexLock
......................................................................
[common] get rid of MutexLock
Since contemporary STL library provides both std::lock_guard and
std::unique_lock, there is no need to keep MutexLock.
Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/master/auto_leader_rebalancer.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/rpc/service_pool.cc
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/service_queue.h
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/table_scanner.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_service.h
M src/kudu/util/async_logger.cc
M src/kudu/util/async_logger.h
M src/kudu/util/barrier.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
M src/kudu/util/cloud/instance_detector.cc
M src/kudu/util/countdown_latch.h
M src/kudu/util/debug/trace_event_impl.cc
M src/kudu/util/debug/trace_event_synthetic_delay.cc
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
M src/kudu/util/mutex.h
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/rwc_lock.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
33 files changed, 155 insertions(+), 195 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/21415/1
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
[kudu-CR] [common] get rid of MutexLock
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2:
> Patch Set 2: Code-Review+1
>
> My only concern/nit about this change is wether we're loosing a bit of code readability, many places a simple "lock" is used for name, and the information about the type of the lock is a bit harder to find out (e.g. have to look up the header file and find the lock declaration to see the lock type).
> It's not a huge issue, and renaming every instance to more specific names seems excessive, just wanted to bring it up and hear your thoughts.
I think I have to clarify a bit, I meant that using 'decltype(insert-lock-name)' hides the lock type information, but I might be just splitting hairs here :)
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 May 2024 08:47:45 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 4:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/144/
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 16:47:05 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2:
> > Patch Set 2: Code-Review+1
> >
> > My only concern/nit about this change is wether we're loosing a
> bit of code readability, many places a simple "lock" is used for
> name, and the information about the type of the lock is a bit
> harder to find out (e.g. have to look up the header file and find
> the lock declaration to see the lock type).
> > It's not a huge issue, and renaming every instance to more
> specific names seems excessive, just wanted to bring it up and hear
> your thoughts.
>
> I think I have to clarify a bit, I meant that using
> 'decltype(insert-lock-name)' hides the lock type information, but I
> might be just splitting hairs here :)
That information in one click away if using an IDE (or one key-press away if using YCM or other editors plugins). Yes, it's no longer
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 05:25:36 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/122/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Thu, 09 May 2024 05:39:26 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2:
> Patch Set 2: Code-Review+1
>
> My only concern/nit about this change is wether we're loosing a bit of code readability, many places a simple "lock" is used for name, and the information about the type of the lock is a bit harder to find out (e.g. have to look up the header file and find the lock declaration to see the lock type).
> It's not a huge issue, and renaming every instance to more specific names seems excessive, just wanted to bring it up and hear your thoughts.
I think I have to clarify a bit, I meant that using 'decltype(insert-lock-name)' hides the lock type information, but I might be just splitting hairs here :)
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 May 2024 08:47:45 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 3:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/143/
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 06:41:33 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 03:32:20 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
[common] get rid of MutexLock
Since contemporary STL library provides both std::lock_guard and
std::unique_lock, there is no need to keep MutexLock.
Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Reviewed-on: http://gerrit.cloudera.org:8080/21415
Tested-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/master/auto_leader_rebalancer.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/rpc/service_pool.cc
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/service_queue.h
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/table_scanner.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_service.h
M src/kudu/util/async_logger.cc
M src/kudu/util/async_logger.h
M src/kudu/util/barrier.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
M src/kudu/util/cloud/instance_detector.cc
M src/kudu/util/countdown_latch.h
M src/kudu/util/debug/trace_event_impl.cc
M src/kudu/util/debug/trace_event_synthetic_delay.cc
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
M src/kudu/util/mutex.h
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/rwc_lock.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
33 files changed, 146 insertions(+), 197 deletions(-)
Approvals:
Alexey Serbin: Verified
Abhishek Chennaka: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [common] get rid of MutexLock
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/127/
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Thu, 09 May 2024 20:42:41 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/127/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Thu, 09 May 2024 22:06:07 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2:
> > > Patch Set 2: Code-Review+1
> > >
> > > My only concern/nit about this change is wether we're loosing a
> > bit of code readability, many places a simple "lock" is used for
> > name, and the information about the type of the lock is a bit
> > harder to find out (e.g. have to look up the header file and find
> > the lock declaration to see the lock type).
> > > It's not a huge issue, and renaming every instance to more
> > specific names seems excessive, just wanted to bring it up and
> hear
> > your thoughts.
> >
> > I think I have to clarify a bit, I meant that using
> > 'decltype(insert-lock-name)' hides the lock type information, but
> I
> > might be just splitting hairs here :)
>
> That information in one click away if using an IDE (or one
> key-press away if using YCM or other editors plugins). Yes, it's
> no longer
Yes, the type information is no longer duplicated as it used to be, but I don't see it as something negative. Take a look at other class members used around in the same .cc file: they have information on their type only in the .h header file where they are declared/defined, and everybody just uses the members in the .cc file without any problem.
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 05:43:37 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 22:59:55 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Attila Bukor, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21415
to look at the new patch set (#2).
Change subject: [common] get rid of MutexLock
......................................................................
[common] get rid of MutexLock
Since contemporary STL library provides both std::lock_guard and
std::unique_lock, there is no need to keep MutexLock.
Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/master/auto_leader_rebalancer.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/rpc/service_pool.cc
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/service_queue.h
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/table_scanner.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_service.h
M src/kudu/util/async_logger.cc
M src/kudu/util/async_logger.h
M src/kudu/util/barrier.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
M src/kudu/util/cloud/instance_detector.cc
M src/kudu/util/countdown_latch.h
M src/kudu/util/debug/trace_event_impl.cc
M src/kudu/util/debug/trace_event_synthetic_delay.cc
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
M src/kudu/util/mutex.h
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/rwc_lock.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
33 files changed, 154 insertions(+), 195 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/21415/2
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2: Verified+1
unrelated test failures: raft_consensus-itest and Java tests
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Thu, 09 May 2024 23:35:48 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 2: Code-Review+1
My only concern/nit about this change is wether we're loosing a bit of code readability, many places a simple "lock" is used for name, and the information about the type of the lock is a bit harder to find out (e.g. have to look up the header file and find the lock declaration to see the lock type).
It's not a huge issue, and renaming every instance to more specific names seems excessive, just wanted to bring it up and hear your thoughts.
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 May 2024 08:41:25 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Zoltan Chovan, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21415
to look at the new patch set (#4).
Change subject: [common] get rid of MutexLock
......................................................................
[common] get rid of MutexLock
Since contemporary STL library provides both std::lock_guard and
std::unique_lock, there is no need to keep MutexLock.
Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/master/auto_leader_rebalancer.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/rpc/service_pool.cc
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/service_queue.h
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/table_scanner.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_service.h
M src/kudu/util/async_logger.cc
M src/kudu/util/async_logger.h
M src/kudu/util/barrier.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
M src/kudu/util/cloud/instance_detector.cc
M src/kudu/util/countdown_latch.h
M src/kudu/util/debug/trace_event_impl.cc
M src/kudu/util/debug/trace_event_synthetic_delay.cc
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
M src/kudu/util/mutex.h
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/rwc_lock.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
33 files changed, 146 insertions(+), 197 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/21415/4
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 4: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/144/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 17:51:18 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/122/
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 09 May 2024 05:33:29 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Zoltan Chovan, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21415
to look at the new patch set (#3).
Change subject: [common] get rid of MutexLock
......................................................................
[common] get rid of MutexLock
Since contemporary STL library provides both std::lock_guard and
std::unique_lock, there is no need to keep MutexLock.
Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/master/auto_leader_rebalancer.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/rpc/service_pool.cc
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/service_queue.h
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/table_scanner.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_service.h
M src/kudu/util/async_logger.cc
M src/kudu/util/async_logger.h
M src/kudu/util/barrier.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
M src/kudu/util/cloud/instance_detector.cc
M src/kudu/util/countdown_latch.h
M src/kudu/util/debug/trace_event_impl.cc
M src/kudu/util/debug/trace_event_synthetic_delay.cc
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
M src/kudu/util/mutex.h
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/rwc_lock.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
33 files changed, 144 insertions(+), 197 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/21415/3
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [common] get rid of MutexLock
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [common] get rid of MutexLock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 4: Verified+1
unrelated test failures, posted a fix to address at least one of them: https://gerrit.cloudera.org/#/c/21427/
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 21:21:02 +0000
Gerrit-HasComments: No
[kudu-CR] [common] get rid of MutexLock
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21415 )
Change subject: [common] get rid of MutexLock
......................................................................
Patch Set 3: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/143/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49e0ef2c688ef8be74d018bb9bffe70b6655e654
Gerrit-Change-Number: 21415
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2024 07:33:25 +0000
Gerrit-HasComments: No