[kudu-CR] [master] allow setting BlockCacheMetrics twice
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 ) Change subject: [master] allow setting BlockCacheMetrics twice .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h File src/kudu/util/cache.h: http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@69 PS2, Line 69: enum > nit (bikeshedding): any specific reason not to use 'enum class' here? Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@69 PS2, Line 69: class ExistingMetric > nit (bikeshedding): maybe, name this ExistingMetricsPolicy ? Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@76 PS2, Line 76: kKeep, > nit (bikeshedding): maybe, name this kKeep (assuming the enum is called Exi Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@81 PS2, Line 81: kReset, > nit (bikeshedding): what that 'Force' stands for semantically? Could 'kRes Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@85 PS2, Line 85: metrics_policy) > Should it be kReset by default? I kept it as kKeep since that's the current behavior in all cases. As for default arguments in pure virtual methods, I've been advised against it before, so I'll keep it as is: https://stackoverflow.com/questions/12139786/good-practice-default-arguments-for-pure-virtual-method http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.cc File src/kudu/util/cache.cc: http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.cc@578 PS2, Line 578: // probably better than spurious failures. > Does it make sense to leave CHECK(IsGTest()) under this 'if()' clause, sinc Done -- To view, visit http://gerrit.cloudera.org:8080/17636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 Gerrit-Change-Number: 17636 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 26 Jun 2021 06:39:42 + Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17636 to look at the new patch set (#3). Change subject: [master] allow setting BlockCacheMetrics twice .. [master] allow setting BlockCacheMetrics twice In some cases, the way we call Cache::SetMetrics() isn't threadsafe. Typically, we only call this once per cache, but this can be a useful call to repeat, e.g. if we're constructing a new server that instantiates its own metric entity but uses the same cache. This usually doesn't happen since most Caches are tied to a single server. However, the BlockCache is special in that it's a singleton, so every instantiation of a server in the same process will use the same cache. This was the cause of KUDU-2165, which was resolved by only allowing the first caller to set the metrics. While it prevented a race, this meant that subsequently started servers would not correctly register metrics. I have an upcoming patch that will start two servers from the same process -- effectively restarting a master after running a tablet copy from the same process as a means to automate the addition of a master to a cluster. To allow this, this patch adds an option to force the resetting of the cache metrics. Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 --- M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master_options.cc M src/kudu/master/master_options.h M src/kudu/master/table_locations_cache.cc M src/kudu/util/cache-test.cc M src/kudu/util/cache.cc M src/kudu/util/cache.h M src/kudu/util/file_cache.cc M src/kudu/util/nvm_cache.cc M src/kudu/util/ttl_cache.h 13 files changed, 98 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/17636/3 -- To view, visit http://gerrit.cloudera.org:8080/17636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 Gerrit-Change-Number: 17636 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [rest] add oat++ framework to kudu
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/17554 )
Change subject: [rest] add oat++ framework to kudu
..
Patch Set 16:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatpp.cmake
File cmake_modules/FindOatpp.cmake:
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatpp.cmake@24
PS16, Line 24: oatpp-1.2.5
Is there a way to use version-agnostic path here? Using version-specific path
might be very inconvenient when the package is updated for a new version in
future.
I'd expect these path to point to files placed under
$KUDU_ROOT/thirdparty/installed/{common,tsan,uninstrumented}, so no version
would be in the path?
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatpp.cmake@29
PS16, Line 29: oatpp-1.2.5
ditto
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatpp.cmake@31
PS16, Line 31: oatpp-1.2.5/
ditto
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatppSwagger.cmake
File cmake_modules/FindOatppSwagger.cmake:
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatppSwagger.cmake@24
PS16, Line 24: oatpp-1.2.5
ditto: is it possible to switch to a version-agnostic path here?
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatppSwagger.cmake@29
PS16, Line 29: oatpp-1.2.5
ditto
http://gerrit.cloudera.org:8080/#/c/17554/16/cmake_modules/FindOatppSwagger.cmake@31
PS16, Line 31: oatpp-1.2.5
ditto
http://gerrit.cloudera.org:8080/#/c/17554/16/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/17554/16/thirdparty/build-definitions.sh@1101
PS16, Line 1101: cmake \
: -DCMAKE_BUILD_TYPE=release \
: -DCMAKE_INSTALL_PREFIX=$PREFIX \
: -DBUILD_SHARED_LIBS=OFF \
: -DOATPP_DISABLE_ENV_OBJECT_COUNTERS=ON \
: -DOATPP_BUILD_TESTS=OFF \
: $OATPP_SOURCE
style nit: for line continuations, add an extra indent (see other build_xxx()
functions in this file for reference)
http://gerrit.cloudera.org:8080/#/c/17554/16/thirdparty/build-definitions.sh@1117
PS16, Line 1117: cmake \
: -DCMAKE_BUILD_TYPE=release \
: -DCMAKE_INSTALL_PREFIX=$PREFIX \
: -DBUILD_SHARED_LIBS=OFF \
: -DOATPP_BUILD_TESTS=OFF \
: -DOATPP_INSTALL=ON \
: $OATPP_SWAGGER_SOURCE
ditto
--
To view, visit http://gerrit.cloudera.org:8080/17554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1b5376297b0170a624655acf836cdedc090f6e7
Gerrit-Change-Number: 17554
Gerrit-PatchSet: 16
Gerrit-Owner: Khazar Mammadli
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 26 Jun 2021 00:20:20 +
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] fix building crcutil with GCC 10
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17626 ) Change subject: [thirdparty] fix building crcutil with GCC 10 .. Patch Set 1: > The change looks good to me. I don't have a strong opinion on the > ordering of patching. Thank you for the review! I already posted a pull request for the crcutil repo athttps://github.com/cloudera/crcutil/pull/4 I hope to get that merged, and then I'll need to update this patch to use the newer snapshot of the crcutil package. -- To view, visit http://gerrit.cloudera.org:8080/17626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibaba794ce00eb64a29e8038a8a2f4883f35c975a Gerrit-Change-Number: 17626 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 26 Jun 2021 00:10:43 + Gerrit-HasComments: No
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 ) Change subject: [master] allow setting BlockCacheMetrics twice .. Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h File src/kudu/util/cache.h: http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@69 PS2, Line 69: MetricsResetBehavior nit (bikeshedding): maybe, name this ExistingMetricsPolicy ? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@69 PS2, Line 69: enum nit (bikeshedding): any specific reason not to use 'enum class' here? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@76 PS2, Line 76: kKeepFirstMetrics nit (bikeshedding): maybe, name this kKeep (assuming the enum is called ExistingMetricsPolicy)? Or the idea is to have snapshots of various metrics in the future? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@81 PS2, Line 81: kForceResetMetrics nit (bikeshedding): what that 'Force' stands for semantically? Could 'kReset' be a good enough name for this (assuming the enum is called ExistingMetricsPolicy)? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@85 PS2, Line 85: metrics_behavior Should it be kReset by default? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.cc File src/kudu/util/cache.cc: http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.cc@578 PS2, Line 578: return; Does it make sense to leave CHECK(IsGTest()) under this 'if()' clause, since (1) we expect this method to be called with kKeep only from tests (2) not setting the provided metrics might point to an issue in non-test environments? -- To view, visit http://gerrit.cloudera.org:8080/17636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 Gerrit-Change-Number: 17636 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 23:44:22 + Gerrit-HasComments: Yes
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. Patch Set 2: > Patch Set 2: > > > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > > > warning? > > > > > > The prior version of the method would take a reference to > > > > > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > > > warning? > > > > > > Thank you for looking at this. > > > > > > I quickly looked at the code around when putting together this > > > patch and I was under impression that 'timeout' was passed by > > > reference into the callback functor which on its turn was passed > > > into LookupTabletByKey(), so I suspected a functor being run with > > > de-allocated MonoTime. > > > > > > After taking another look, I found turns out the 'timeout' is > > used > > > to compute 'deadline' using 'timeout' right before doing anything > > > else, and then 'deadline' is actually passed around by _value_ in > > > CoordinateTransactionAsync(). > > > > > > So yes, it seems this might be a dud fix, but it should not hurt > > in > > > any other way, and we pass MonoDelta by value in many more other > > > cases anyway, so you consider this just as a 'style' fix :) > > > > > ... so you consider this just as a 'style' fix :) > > > > I meant "... so you can consider this to be just a 'style" fix :)" > > After another look, it seems the issue here is about passing > MonoDelta::FromNanoseconds(std::numeric_limits::max()) as a value > for 'timeout' parameter when calling > TxnSystemClient::CoordinateTransactionAsync() Ah indeed, I assumed this was a case of the MonoDelta being used in an async function but being destructed before actually being used. Good call-out Bankim. -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 23:22:53 + Gerrit-HasComments: No
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 ) Change subject: [master] allow setting BlockCacheMetrics twice .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc File src/kudu/master/master_options.cc: http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc@59 PS1, Line 59: } > Nit: This could be part of the constructor initliazer list Done -- To view, visit http://gerrit.cloudera.org:8080/17636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 Gerrit-Change-Number: 17636 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 23:10:14 + Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17636 to look at the new patch set (#2). Change subject: [master] allow setting BlockCacheMetrics twice .. [master] allow setting BlockCacheMetrics twice In some cases, the way we call Cache::SetMetrics() isn't threadsafe. Typically, we only call this once per cache, but this can be a useful call to repeat, e.g. if we're constructing a new server that instantiates its own metric entity but uses the same cache. This usually doesn't happen since most Caches are tied to a single server. However, the BlockCache is special in that it's a singleton, so every instantiation of a server in the same process will use the same cache. This was the cause of KUDU-2165, which was resolved by only allowing the first caller to set the metrics. While it prevented a race, this meant that subsequently started servers would not correctly register metrics. I have an upcoming patch that will start two servers from the same process -- effectively restarting a master after running a tablet copy from the same process as a means to automate the addition of a master to a cluster. To allow this, this patch adds an option to force the resetting of the cache metrics. Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 --- M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master_options.cc M src/kudu/master/master_options.h M src/kudu/master/table_locations_cache.cc M src/kudu/util/cache-test.cc M src/kudu/util/cache.cc M src/kudu/util/cache.h M src/kudu/util/file_cache.cc M src/kudu/util/nvm_cache.cc M src/kudu/util/ttl_cache.h 13 files changed, 98 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/17636/2 -- To view, visit http://gerrit.cloudera.org:8080/17636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 Gerrit-Change-Number: 17636 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. Patch Set 2: > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > > warning? > > > > The prior version of the method would take a reference to > > > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > > warning? > > > > Thank you for looking at this. > > > > I quickly looked at the code around when putting together this > > patch and I was under impression that 'timeout' was passed by > > reference into the callback functor which on its turn was passed > > into LookupTabletByKey(), so I suspected a functor being run with > > de-allocated MonoTime. > > > > After taking another look, I found turns out the 'timeout' is > used > > to compute 'deadline' using 'timeout' right before doing anything > > else, and then 'deadline' is actually passed around by _value_ in > > CoordinateTransactionAsync(). > > > > So yes, it seems this might be a dud fix, but it should not hurt > in > > any other way, and we pass MonoDelta by value in many more other > > cases anyway, so you consider this just as a 'style' fix :) > > > ... so you consider this just as a 'style' fix :) > > I meant "... so you can consider this to be just a 'style" fix :)" After another look, it seems the issue here is about passing MonoDelta::FromNanoseconds(std::numeric_limits::max()) as a value for 'timeout' parameter when calling TxnSystemClient::CoordinateTransactionAsync() -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 21:52:40 + Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. Patch Set 2: > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > warning? > > The prior version of the method would take a reference to > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN > > warning? > > Thank you for looking at this. > > I quickly looked at the code around when putting together this > patch and I was under impression that 'timeout' was passed by > reference into the callback functor which on its turn was passed > into LookupTabletByKey(), so I suspected a functor being run with > de-allocated MonoTime. > > After taking another look, I found turns out the 'timeout' is used > to compute 'deadline' using 'timeout' right before doing anything > else, and then 'deadline' is actually passed around by _value_ in > CoordinateTransactionAsync(). > > So yes, it seems this might be a dud fix, but it should not hurt in > any other way, and we pass MonoDelta by value in many more other > cases anyway, so you consider this just as a 'style' fix :) > ... so you consider this just as a 'style' fix :) I meant "... so you can consider this to be just a 'style" fix :)" -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 21:31:38 + Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. Patch Set 2: > Sorry I didn't understand. How does pass-by-value fix the UBSAN > warning? The prior version of the method would take a reference to > Sorry I didn't understand. How does pass-by-value fix the UBSAN > warning? Thank you for looking at this. I quickly looked at the code around when putting together this patch and I was under impression that 'timeout' was passed by reference into the callback functor which on its turn was passed into LookupTabletByKey(), so I suspected a functor being run with de-allocated MonoTime. After taking another look, I found turns out the 'timeout' is used to compute 'deadline' using 'timeout' right before doing anything else, and then 'deadline' is actually passed around by _value_ in CoordinateTransactionAsync(). So yes, it seems this might be a dud fix, but it should not hurt in any other way, and we pass MonoDelta by value in many more other cases anyway, so you consider this just as a 'style' fix :) -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 21:24:46 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] fix building crcutil with GCC 10
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17626 ) Change subject: [thirdparty] fix building crcutil with GCC 10 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibaba794ce00eb64a29e8038a8a2f4883f35c975a Gerrit-Change-Number: 17626 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 20:54:56 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] fix building crcutil with GCC 10
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17626 ) Change subject: [thirdparty] fix building crcutil with GCC 10 .. Patch Set 1: The change looks good to me. I don't have a strong opinion on the ordering of patching. -- To view, visit http://gerrit.cloudera.org:8080/17626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibaba794ce00eb64a29e8038a8a2f4883f35c975a Gerrit-Change-Number: 17626 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 20:54:51 + Gerrit-HasComments: No
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 ) Change subject: [master] allow setting BlockCacheMetrics twice .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc File src/kudu/master/master_options.cc: http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc@59 PS1, Line 59: block_cache_metrics_reset_behavior_ = Cache::kKeepFirstMetrics; Nit: This could be part of the constructor initliazer list -- To view, visit http://gerrit.cloudera.org:8080/17636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 Gerrit-Change-Number: 17636 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 20:29:38 + Gerrit-HasComments: Yes
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. Patch Set 2: Sorry I didn't understand. How does pass-by-value fix the UBSAN warning? -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 19:17:09 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] fix building LLVM 9 with GCC 10
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17627 ) Change subject: [thirdparty] fix building LLVM 9 with GCC 10 .. [thirdparty] fix building LLVM 9 with GCC 10 This patch fixes LLVM 9 build with GCC 10 in Kudu's thirdparty. The imported patch is already in the LLVM repo, see [1]. Prior to this patch, LLVM9 build in thirdparty would fail with the following error: thirdparty/src/llvm-9.0.0.src/include/llvm/Demangle/MicrosoftDemangleNodes.h:566:3: error: unknown type name 'uint32_t' uint32_t NVOffset = 0; ^ [1] https://github.com/llvm-mirror/llvm/commit/e0402b5c9813a2458b8dd3f640883110db280395 Change-Id: I3332556229dbf2bce65d52f37c48d62624465f70 Reviewed-on: http://gerrit.cloudera.org:8080/17627 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M thirdparty/download-thirdparty.sh A thirdparty/patches/llvm-MicrosoftDemangleNodes-e0402b5c9813a2458b8dd3f640883110db280395.patch 2 files changed, 34 insertions(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3332556229dbf2bce65d52f37c48d62624465f70 Gerrit-Change-Number: 17627 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() This patch updates the signature of the TxnSystemClient::CoordinateTransactionAsync() method to pass MonoDelta by value. In addition, I added an extra DCHECK() into MonoTime::AddDelta(). The motivation for this change was seeing the following UBSAN warning in one of the pre-commit builds [1]: #0 0x7f48d11e8f3c in kudu::MonoTime::AddDelta(kudu::MonoDelta const&) src/kudu/util/monotime.cc:218:10 #1 0x7f48d11e9eee in kudu::operator+(kudu::MonoTime const&, kudu::MonoDelta const&) src/kudu/util/monotime.cc:333:7 #2 0x7f48e0d846c1 in kudu::transactions::TxnSystemClient::CoordinateTransactionAsync(kudu::tserver::CoordinatorOpPB, kudu::MonoDelta const&, std::function const&, kudu::tserver::CoordinatorOpResultPB*) src/kudu/transactions/txn_system_client.cc:331:45 #3 0x7f48e0d86f76 in kudu::transactions::TxnSystemClient::KeepTransactionAlive(long, std::__cxx11::basic_string, std::allocator > const&, kudu::MonoDelta) src/kudu/transactions/txn_system_client.cc:320:3 #4 0x7f48e24c62b9 in kudu::transactions::TxnManager::KeepTransactionAlive(long, std::__cxx11::basic_string, std::allocator > const&, kudu::MonoTime const&) src/kudu/master/txn_manager.cc:238:27 #5 0x7f48e24ca36f in kudu::transactions::TxnManagerServiceImpl::KeepTransactionAlive(kudu::transactions::KeepTransactionAliveRequestPB const*, kudu::transactions::KeepTransactionAliveResponsePB*, kudu::rpc::RpcContext*) src/kudu/master/txn_manager_service.cc:159:42 #6 0x7f48d7224b0e in std::function::operator()(google::protobuf::Message const*, google::protobuf::Message*, kudu::rpc::RpcContext*) const ../../../include/c++/7.5.0/bits/std_function.h:706:14 #7 0x7f48d7223afc in kudu::rpc::GeneratedServiceIf::Handle(kudu::rpc::InboundCall*) src/kudu/rpc/service_if.cc:137:3 #8 0x7f48d7229a9d in kudu::rpc::ServicePool::RunThread() src/kudu/rpc/service_pool.cc:232:15 #9 0x7f48d1290c3a in kudu::Thread::SuperviseThread(void*) src/kudu/util/thread.cc:674:3 #10 0x7f48d3a046da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da) #11 0x7f48cd55771e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12171e) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/kudu/util/monotime.cc:218:10 [1] http://dist-test.cloudera.org/job?job_id=jenkins-slave.1623914260.1110749 Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Reviewed-on: http://gerrit.cloudera.org:8080/17611 Tested-by: Kudu Jenkins Reviewed-by: Abhishek Chennaka Reviewed-by: Andrew Wong --- M src/kudu/transactions/txn_system_client.cc M src/kudu/transactions/txn_system_client.h M src/kudu/util/monotime.cc 3 files changed, 3 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Abhishek Chennaka: Looks good to me, but someone else must approve Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thirdparty] fix building LLVM 9 with GCC 10
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17627 ) Change subject: [thirdparty] fix building LLVM 9 with GCC 10 .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3332556229dbf2bce65d52f37c48d62624465f70 Gerrit-Change-Number: 17627 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 17:54:48 + Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 17:52:43 + Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 ) Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync() .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/17611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d Gerrit-Change-Number: 17611 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 17:51:51 + Gerrit-HasComments: No
