[kudu-CR] KUDU-2411: Add OS/Arch detection to binary extract
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12141 ) Change subject: KUDU-2411: Add OS/Arch detection to binary extract .. Patch Set 5: (3 comments) Looking good, I just have some minor comments http://gerrit.cloudera.org:8080/#/c/12141/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java: http://gerrit.cloudera.org:8080/#/c/12141/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java@88 PS5, Line 88: "Set the system variable " + KUDU_BIN_DIR_PROP + please update this message to include something like: "Set the system variable " + KUDU_BIN_DIR_PROP + " or add the Kudu binary test jar to your classpath or ensure the `kudu` binary is on your path." http://gerrit.cloudera.org:8080/#/c/12141/5/java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java File java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java: http://gerrit.cloudera.org:8080/#/c/12141/5/java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java@93 PS5, Line 93: customize nit: Just a style guide thing, please capitalize the first word in the sentence in a comment. Also please end the sentence with punctuation such as a period. http://gerrit.cloudera.org:8080/#/c/12141/5/java/kudu-test-utils/src/test/resources/log4j.properties File java/kudu-test-utils/src/test/resources/log4j.properties: http://gerrit.cloudera.org:8080/#/c/12141/5/java/kudu-test-utils/src/test/resources/log4j.properties@24 PS5, Line 24: log4j.logger.com.google.gradle = INFO Please add a comment as to why this is necessary / useful -- To view, visit http://gerrit.cloudera.org:8080/12141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9752914a426dd1572610f891dad4d4778d04f79c Gerrit-Change-Number: 12141 Gerrit-PatchSet: 5 Gerrit-Owner: Brian McDevitt Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 25 Jan 2019 07:32:23 + Gerrit-HasComments: Yes
[kudu-CR] Fix master sentry-itest built in RELEASE mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12274 ) Change subject: Fix master_sentry-itest built in RELEASE mode .. Patch Set 1: (1 comment) > (1 comment) > > > It is necessary to not break macOS build. > > So the macOS build is already broken? Or is it somehow broken from > this change? > > If the latter, I don't understand why; we've been building > sentry_authz_provider.cc on macOS for some time. It is the latter, after I add a to SentryAuthzProvider in CatalogManager, > (1 comment) > > > It is necessary to not break macOS build. > > So the macOS build is already broken? Or is it somehow broken from > this change? > > If the latter, I don't understand why; we've been building > sentry_authz_provider.cc on macOS for some time. Right, once I added the call to SentryAuthzProvider in CatalogManager, the compilation on macOS failed with: In file included from /Users/hao.hao/Documents/git-repos/kudu/src/kudu/master/catalog_manager.cc:100: In file included from /Users/hao.hao/Documents/git-repos/kudu/src/kudu/master/sentry_authz_provider.h:24: In file included from /Users/hao.hao/Documents/git-repos/kudu/src/kudu/sentry/sentry_client.h:23: In file included from /Users/hao.hao/Documents/git-repos/kudu/build/debug/src/kudu/sentry/SentryPolicyService.h:12: /Users/hao.hao/Documents/git-repos/kudu/build/debug/src/kudu/sentry/sentry_policy_service_types.h:26:5: error: expected identifier TRUE = 1, ^ /usr/include/mach/boolean.h:81:14: note: expanded from macro 'TRUE' #define TRUE1 http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc@747 PS1, Line 747: authz_provider_.reset(new master::SentryAuthzProvider()); > I thought when the full Sentry integration is merged, Start/Stop will be ca Sure, I can change to that. -- To view, visit http://gerrit.cloudera.org:8080/12274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4e90b2dc5372c87b78d381f04780f6b5db60271c Gerrit-Change-Number: 12274 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jan 2019 06:24:23 + Gerrit-HasComments: Yes
[kudu-CR] Fix master sentry-itest built in RELEASE mode
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12274 ) Change subject: Fix master_sentry-itest built in RELEASE mode .. Patch Set 1: (1 comment) > It is necessary to not break macOS build. So the macOS build is already broken? Or is it somehow broken from this change? If the latter, I don't understand why; we've been building sentry_authz_provider.cc on macOS for some time. http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc@747 PS1, Line 747: authz_provider_.reset(new master::SentryAuthzProvider()); > Right, the constructor of SentryAuthzProvider isn't doing anything. I thought when the full Sentry integration is merged, Start/Stop will be called unconditionally, and the gflag will control whether we construct a SentryAuthzProvider or a DefaultAuthzProvider? Since you're doing everything but Start/Stop in this patch, why not use that gflag control now too? -- To view, visit http://gerrit.cloudera.org:8080/12274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4e90b2dc5372c87b78d381f04780f6b5db60271c Gerrit-Change-Number: 12274 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jan 2019 05:35:15 + Gerrit-HasComments: Yes
[kudu-CR] generic iterators: prep for MergeIterator dominance
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12196 ) Change subject: generic_iterators: prep for MergeIterator dominance .. generic_iterators: prep for MergeIterator dominance This patch adds a counter to the MergeIterator that tracks the number of comparisons made during the lifetime of the iterator. When the dominance algorithm is added, the counter's value ought to drop. TestMerge now logs the counter's value, and can also generate non-overlapped inputs if desired. The counter isn't exposed to users. It could be added to IteratorStats and set for every key column, but even then it'll only apply to ORDERED scans, so the value is dubious. Change-Id: I3a8db5973dc09b7271e05b3cc28025b7a2a9e21b Reviewed-on: http://gerrit.cloudera.org:8080/12196 Reviewed-by: Mike Percy Tested-by: Kudu Jenkins --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/generic_iterators.h 3 files changed, 49 insertions(+), 7 deletions(-) Approvals: Mike Percy: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3a8db5973dc09b7271e05b3cc28025b7a2a9e21b Gerrit-Change-Number: 12196 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Fix master sentry-itest built in RELEASE mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12274 ) Change subject: Fix master_sentry-itest built in RELEASE mode .. Patch Set 1: (1 comment) > (1 comment) > > The changes to sentry_authz_provider and the thrift file seem > unrelated. It is necessary to not break macOS build. http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc@747 PS1, Line 747: authz_provider_.reset(new master::SentryAuthzProvider()); > Shouldn't this be conditioned on some Sentry gflag? Or is that conditioning Right, the constructor of SentryAuthzProvider isn't doing anything. -- To view, visit http://gerrit.cloudera.org:8080/12274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4e90b2dc5372c87b78d381f04780f6b5db60271c Gerrit-Change-Number: 12274 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jan 2019 05:23:34 + Gerrit-HasComments: Yes
[kudu-CR] Fix master sentry-itest built in RELEASE mode
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12274 ) Change subject: Fix master_sentry-itest built in RELEASE mode .. Patch Set 1: (1 comment) The changes to sentry_authz_provider and the thrift file seem unrelated. http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12274/1/src/kudu/master/catalog_manager.cc@747 PS1, Line 747: authz_provider_.reset(new master::SentryAuthzProvider()); Shouldn't this be conditioned on some Sentry gflag? Or is that conditioning only necessary for Start()/Stop() on authz_provider_? -- To view, visit http://gerrit.cloudera.org:8080/12274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4e90b2dc5372c87b78d381f04780f6b5db60271c Gerrit-Change-Number: 12274 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jan 2019 05:14:23 + Gerrit-HasComments: Yes
[kudu-CR] generic iterators: move iterator declarations into cc file
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12223 ) Change subject: generic_iterators: move iterator declarations into cc file .. generic_iterators: move iterator declarations into cc file I plan on converting MergeIterator's state vector into an intrusive list, which means MergeIterState's class definition needs to be visible to MergeIterator's class definition. Instead of moving MergeIterState into the header, let's go the other way and move all of the iterator declarations into the .cc file. The code outside of these classes doesn't care about the concrete types, and it should speed up compilation a bit. I had to poke a hole in for some PredicateEvaluatingIterator tests though. Change-Id: I301f39ec0d55b73cadcf28d8104accca3219ab1b Reviewed-on: http://gerrit.cloudera.org:8080/12223 Reviewed-by: Mike Percy Tested-by: Kudu Jenkins --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/generic_iterators.h M src/kudu/tablet/cfile_set-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/rowset.cc M src/kudu/tablet/tablet.cc 9 files changed, 345 insertions(+), 316 deletions(-) Approvals: Mike Percy: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I301f39ec0d55b73cadcf28d8104accca3219ab1b Gerrit-Change-Number: 12223 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [sentry] Integrate AuthzProvider into CatalogManager
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 ) Change subject: [sentry] Integrate AuthzProvider into CatalogManager .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@753 PS3, Line 753: hms::HmsCatalog::IsEnabled() > Isn't this already guaranteed by L726? Done http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/sentry/sentry_policy_service.thrift File src/kudu/sentry/sentry_policy_service.thrift: http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/sentry/sentry_policy_service.thrift@26 PS3, Line 26: # - Rename enum TSentryGrantOption.TRUE and TSentryGrantOption.FALSE to avoid : # conflict with the macro definition in the system header. > This rename is safe because only the value of the enum is sent on the wire, Yes, that's my understanding. -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 25 Jan 2019 02:23:03 + Gerrit-HasComments: Yes
[kudu-CR] Fix master sentry-itest built in RELEASE mode
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12274 Change subject: Fix master_sentry-itest built in RELEASE mode .. Fix master_sentry-itest built in RELEASE mode Commit 9098f442e introduced a dummy integration test to ensure mini cluster can start with both mini sentry and mini hms. However, this test fails with 'unknown command line flag' error in RELEASE mode. Since RELEASE builds are statically linked, the linker drops the symbols defined by 'sentry_authz_provider.cc', considering they're not needed. Because there is no static call path from the master into any function in 'sentry_authz_provider.cc'. This test passed upstream pre-commit check because we force dynamic linkage in all dist-test runs, even for RELEASE builds. This patch fixes it by adding such call path explicitly. A proper integration with SentryAuthzProvider in the master will be landed in a follow-up. master_sentry-itest built in RELEASE mode passed locally after the change. Change-Id: I4e90b2dc5372c87b78d381f04780f6b5db60271c --- M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/sentry/sentry_policy_service.thrift 5 files changed, 27 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12274/1 -- To view, visit http://gerrit.cloudera.org:8080/12274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4e90b2dc5372c87b78d381f04780f6b5db60271c Gerrit-Change-Number: 12274 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
[kudu-CR] generic iterators: move iterator declarations into cc file
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12223 ) Change subject: generic_iterators: move iterator declarations into cc file .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I301f39ec0d55b73cadcf28d8104accca3219ab1b Gerrit-Change-Number: 12223 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 25 Jan 2019 00:55:59 + Gerrit-HasComments: No
[kudu-CR] generic iterators: prep for MergeIterator dominance
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12196 ) Change subject: generic_iterators: prep for MergeIterator dominance .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a8db5973dc09b7271e05b3cc28025b7a2a9e21b Gerrit-Change-Number: 12196 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 25 Jan 2019 01:00:30 + Gerrit-HasComments: No
[kudu-CR] generic iterators: move iterator declarations into cc file
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12223 ) Change subject: generic_iterators: move iterator declarations into cc file .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I301f39ec0d55b73cadcf28d8104accca3219ab1b Gerrit-Change-Number: 12223 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 25 Jan 2019 01:00:12 + Gerrit-HasComments: No
[kudu-CR] [java] deflake tests that use KuduTestHarness.findLeaderMasterServer
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12263 ) Change subject: [java] deflake tests that use KuduTestHarness.findLeaderMasterServer .. [java] deflake tests that use KuduTestHarness.findLeaderMasterServer >From time to time I'd see test failures like these: 10:10:16.018 [INFO - Test worker] (KuduTestHarness.java:147) Creating a new Kudu client... ... 10:10:16.036 [WARN - New I/O worker #158] (ConnectToCluster.java:278) None of the provided masters 127.6.239.254:42291,127.6.239.252:41769,127.6.239.253:41053 is a leader; will retry ... 10:10:16.060 [ERROR - Test worker] (RetryRule.java:80) testExportAuthenticationCredentialsDuringLeaderElection(org.apache.kudu.client.TestKuduClient): failed attempt 1 org.apache.kudu.client.NoLeaderFoundException: Master config (127.6.239.254:42291,127.6.239.252:41769,127.6.239.253:41053) has no leader. at org.apache.kudu.client.ConnectToCluster.incrementCountAndCheckExhausted(ConnectToCluster.java:279) at org.apache.kudu.client.ConnectToCluster.access$100(ConnectToCluster.java:47) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:323) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:312) at com.stumbleupon.async.Deferred.doCall(Deferred.java:1280) at com.stumbleupon.async.Deferred.runCallbacks(Deferred.java:1259) at com.stumbleupon.async.Deferred.callback(Deferred.java:1002) at org.apache.kudu.client.KuduRpc.handleCallback(KuduRpc.java:247) at org.apache.kudu.client.KuduRpc.callback(KuduRpc.java:294) at org.apache.kudu.client.RpcProxy.responseReceived(RpcProxy.java:269) at org.apache.kudu.client.RpcProxy.access$000(RpcProxy.java:59) at org.apache.kudu.client.RpcProxy$1.call(RpcProxy.java:131) at org.apache.kudu.client.RpcProxy$1.call(RpcProxy.java:127) at org.apache.kudu.client.Connection.messageReceived(Connection.java:391) at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70) at org.apache.kudu.client.Connection.handleUpstream(Connection.java:243) I'd look through the code and wonder how this could happen if KUDU-2387 was indeed fixed. Today I finally noticed that KuduTestHarness.findLeaderMasterServer calls getMasterTableLocationsPB directly, and I remembered that without applying the same logic as in KUDU-2387, such calls will not retry. After adding a catch block to findLeaderMasterServer and transforming the thrown exception, I got a useful stack trace confirming the problem: 10:51:53.627 [ERROR - Test worker] (RetryRule.java:80) testExportAuthenticationCredentialsDuringLeaderElection(org.apache.kudu.client.TestKuduClient): failed attempt 1 org.apache.kudu.client.NoLeaderFoundException: Master config (127.11.27.62:40985,127.11.27.60:37593,127.11.27.61:37931) has no leader. at org.apache.kudu.client.KuduException.transformException(KuduException.java:110) at org.apache.kudu.test.KuduTestHarness.findLeaderMasterServer(KuduTestHarness.java:281) at org.apache.kudu.test.KuduTestHarness.restartLeaderMaster(KuduTestHarness.java:329) at org.apache.kudu.client.TestKuduClient.runTestCallDuringLeaderElection(TestKuduClient.java:1124) at org.apache.kudu.client.TestKuduClient.testExportAuthenticationCredentialsDuringLeaderElection(TestKuduClient.java:1150) ... Suppressed: org.apache.kudu.client.KuduException$OriginalException: Original asynchronous stack trace at org.apache.kudu.client.ConnectToCluster.incrementCountAndCheckExhausted(ConnectToCluster.java:279) at org.apache.kudu.client.ConnectToCluster.access$100(ConnectToCluster.java:47) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:323) at org.apache.kudu.client.ConnectToCluster$ConnectToMasterCB.call(ConnectToCluster.java:312) at com.stumbleupon.async.Deferred.doCall(Deferred.java:1280) at com.stumbleupon.async.Deferred.runCallbacks(Deferred.java:1259) at com.stumbleupon.async.Deferred.callback(Deferred.java:1002) at org.apache.kudu.client.KuduRpc.handleCallback(KuduRpc.java:247) <...netty> This patch fixes these issues by providing an alternate way to find the leader master: if not known, make some call that will only succeed if the leader master is known, then try again. Without the fix, 29/1000 runs of TestKuduClient failed with this error, either in testExportAuthenticationCredentialsDuringLeaderElection or in testGetHiveMetastoreConfigDuringLeaderElection. With the fix, 0/1000 runs of TestKuduClient failed. Change-Id: I5612619d1b9e30df7d627f2370d60ce2aa812713 Reviewed-on: http://gerrit.cloudera.org:8080/12263 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke Reviewed-by: Alexey Serbin --- M
[kudu-CR] switch all iterators to unique ptr
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/1 ) Change subject: switch all iterators to unique_ptr .. switch all iterators to unique_ptr We've suffered from a longstanding wart in the iterator subsystem: a mix of raw and various kinds of smart pointers. The usage of shared_ptr was particularly vexing as it suggested that iterators had shared ownership when in fact they didn't. This yak shave of a patch addresses all of those issues by converting all of the iterator pointer types to unique_ptr. I snuck in some clang-tidy suggestions too, but otherwise the cleanup here should narrowly scoped. Change-Id: I60cdc49e6a209c72cb33681ae1be89586d739204 Reviewed-on: http://gerrit.cloudera.org:8080/1 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy --- M src/kudu/cfile/cfile-test-base.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/generic_iterators.h M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/master/sys_catalog.cc M src/kudu/tablet/all_types-scan-correctness-test.cc M src/kudu/tablet/cfile_set-test.cc M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h M src/kudu/tablet/compaction.cc M src/kudu/tablet/composite-pushdown-test.cc M src/kudu/tablet/delta_compaction-test.cc M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltafile-test.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diff_scan-test.cc M src/kudu/tablet/diskrowset-test-base.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/major_delta_compaction-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/mt-rowset_delta_compaction-test.cc M src/kudu/tablet/mt-tablet-test.cc M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet-decoder-eval-test.cc M src/kudu/tablet/tablet-pushdown-test.cc M src/kudu/tablet/tablet-schema-test.cc M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet-test-util.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_random_access-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 57 files changed, 294 insertions(+), 302 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/1 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I60cdc49e6a209c72cb33681ae1be89586d739204 Gerrit-Change-Number: 1 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: prep for MergeIterator dominance
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12196 ) Change subject: generic_iterators: prep for MergeIterator dominance .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a8db5973dc09b7271e05b3cc28025b7a2a9e21b Gerrit-Change-Number: 12196 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 25 Jan 2019 00:56:11 + Gerrit-HasComments: No
[kudu-CR] [java] deflake tests that use KuduTestHarness.findLeaderMasterServer
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12263 ) Change subject: [java] deflake tests that use KuduTestHarness.findLeaderMasterServer .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5612619d1b9e30df7d627f2370d60ce2aa812713 Gerrit-Change-Number: 12263 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jan 2019 00:55:26 + Gerrit-HasComments: No
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. [fs]: delete the (orphaned) metadata file when repairing With KUDU-2636 fixed, we now support deleting dead containers at runtime. Because this involves deleting a pair of files, there's a time window in which it's possible for there to be just one file. A well-timed crash or kill -9 can make this a permanent state that fails the log block manager at startup. Indeed, a loop of dense_node-itest failed 2/1000 times in this way. This patch fixes the problem by detecting "orphaned" metadata files at startup and deleting them. A metadata file is orphaned if it has no corresponding data file and if it contains nothing but dead blocks. Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Reviewed-on: http://gerrit.cloudera.org:8080/12239 Reviewed-by: Adar Dembo Tested-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 2 files changed, 349 insertions(+), 29 deletions(-) Approvals: Adar Dembo: Looks good to me, approved; Verified Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 9 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 8: (1 comment) LGTM. @Adar, I can merge it if you are also good with it. http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772 PS6, Line 772: con > Won't this case be handled by the existing "delete dead containers at start After more thought on this, I think manual intervention is better than auto repair since it is unclear why could end up in such case. -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Fri, 25 Jan 2019 00:03:32 + Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772 PS6, Line 772: con > Won't this case be handled by the existing "delete dead containers at start Hao and I discussed this on Slack. She pointed to the corresponding test case, where the LBM starts up with Status::Corruption. That's because the data file size was really 0, which meant that the dead blocks described the metadata file were treated as malformed in ProcessRecord(). I overlooked that, thinking that while the _number of bytes on disk_ was 0, the file size was something closer to FLAGS_log_container_max_size. We could address that by deferring block consistency checks until after all DELETEs are processed, thus only checking live blocks. Later, the dead container cleanup code would remove the container. But I don't think that needs to be addressed here, because the goal of this patch is to clean up the so-called "half present" containers, and in that case, the data file is missing outright. I'm not sure under what circumstances someone would end up with a zero-length data file; perhaps they accidentally truncated the data file? Or ran with fsync disabled and created+deleted one block? -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Fri, 25 Jan 2019 00:08:17 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2411: Add OS/Arch detection to binary extract
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12141 ) Change subject: KUDU-2411: Add OS/Arch detection to binary extract .. Patch Set 5: > Patch Set 5: > > One new failure: > > > /home/jenkins-slave/workspace/kudu-master/0/src/kudu/integration-tests/dense_node-itest.cc:217 > Failed > Bad status: Runtime error: > /tmp/dist-test-taskashoUq/build/release/bin/kudu-tserver: process exited on > signal 6 (core dumpe You can ignore that; it's a failure that He Lifu is working on (https://gerrit.cloudera.org/c/12239). -- To view, visit http://gerrit.cloudera.org:8080/12141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9752914a426dd1572610f891dad4d4778d04f79c Gerrit-Change-Number: 12141 Gerrit-PatchSet: 5 Gerrit-Owner: Brian McDevitt Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 24 Jan 2019 23:49:44 + Gerrit-HasComments: No
[kudu-CR] [backup] Use upsert on restore
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12268 ) Change subject: [backup] Use upsert on restore .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12268/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12268/1//COMMIT_MSG@9 PS1, Line 9: us use http://gerrit.cloudera.org:8080/#/c/12268/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala: http://gerrit.cloudera.org:8080/#/c/12268/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@74 PS1, Line 74: // Upsert so that Spark task retries do not fail. I agree that upserts will be necessary when retrying, and for restoring an incremental backup. But could we still use insert when restoring a full backup and not retrying? Insert opens the door for future optimizations like disabling duplicate key checking. With upsert we'll always need to do that (in order to convert the upsert into either an insert or an update). See commit e25348cbb for some past context. -- To view, visit http://gerrit.cloudera.org:8080/12268 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9905fe4301db8c06fe4f18318ccb04b4179a1601 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 24 Jan 2019 23:58:03 + Gerrit-HasComments: Yes
[kudu-CR] switch all iterators to unique ptr
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/1 ) Change subject: switch all iterators to unique_ptr .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/1 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60cdc49e6a209c72cb33681ae1be89586d739204 Gerrit-Change-Number: 1 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 24 Jan 2019 23:39:30 + Gerrit-HasComments: No
[kudu-CR] KUDU-2411: Add OS/Arch detection to binary extract
Brian McDevitt has posted comments on this change. ( http://gerrit.cloudera.org:8080/12141 ) Change subject: KUDU-2411: Add OS/Arch detection to binary extract .. Patch Set 5: One new failure: /home/jenkins-slave/workspace/kudu-master/0/src/kudu/integration-tests/dense_node-itest.cc:217 Failed Bad status: Runtime error: /tmp/dist-test-taskashoUq/build/release/bin/kudu-tserver: process exited on signal 6 (core dumpe -- To view, visit http://gerrit.cloudera.org:8080/12141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9752914a426dd1572610f891dad4d4778d04f79c Gerrit-Change-Number: 12141 Gerrit-PatchSet: 5 Gerrit-Owner: Brian McDevitt Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 24 Jan 2019 23:30:25 + Gerrit-HasComments: No
[kudu-CR] KUDU-2411: Add OS/Arch detection to binary extract
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12141 ) Change subject: KUDU-2411: Add OS/Arch detection to binary extract .. Patch Set 4: > Patch Set 4: > > Jenkins appears to fail on unrelated Python code: > > 14:54:22 Command > "/home/jenkins-slave/workspace/kudu-master/1/build/debug/py_env/bin/python3 > /home/jenkins-slave/workspace/kudu-master/1/build/debug/py_env/lib/python3.4/site-packages/pip/_vendor/pep517/_in_process.py > get_requires_for_build_wheel /tmp/tmpd978xbq3" failed with error code 1 in > /tmp/pip-install-zklrwiys/pandas If you rebase this change onto the tip of master, you'll get a fix for that failure. -- To view, visit http://gerrit.cloudera.org:8080/12141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9752914a426dd1572610f891dad4d4778d04f79c Gerrit-Change-Number: 12141 Gerrit-PatchSet: 4 Gerrit-Owner: Brian McDevitt Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 24 Jan 2019 23:06:10 + Gerrit-HasComments: No
[kudu-CR] KUDU-2411: Add OS/Arch detection to binary extract
Brian McDevitt has posted comments on this change. ( http://gerrit.cloudera.org:8080/12141 ) Change subject: KUDU-2411: Add OS/Arch detection to binary extract .. Patch Set 4: Jenkins appears to fail on unrelated Python code: 14:54:22 Command "/home/jenkins-slave/workspace/kudu-master/1/build/debug/py_env/bin/python3 /home/jenkins-slave/workspace/kudu-master/1/build/debug/py_env/lib/python3.4/site-packages/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /tmp/tmpd978xbq3" failed with error code 1 in /tmp/pip-install-zklrwiys/pandas -- To view, visit http://gerrit.cloudera.org:8080/12141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9752914a426dd1572610f891dad4d4778d04f79c Gerrit-Change-Number: 12141 Gerrit-PatchSet: 4 Gerrit-Owner: Brian McDevitt Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 24 Jan 2019 23:01:19 + Gerrit-HasComments: No
[kudu-CR] KUDU-2411: Add OS/Arch detection to binary extract
Hello Mike Percy, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12141 to look at the new patch set (#4). Change subject: KUDU-2411: Add OS/Arch detection to binary extract .. KUDU-2411: Add OS/Arch detection to binary extract OS/Arch detection ensures that the runtime has the appropriate Kudu binaries to execute the MiniKuduCluster. Currently this limited to OS: osx|linux and Arch: x86_64. Change-Id: I9752914a426dd1572610f891dad4d4778d04f79c --- M java/gradle/dependencies.gradle M java/kudu-test-utils/build.gradle M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryJarExtractor.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/KuduBinaryLocator.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java D java/kudu-test-utils/src/test/resources/fake-kudu-binary/META-INF/apache-kudu-test-binary.properties M java/kudu-test-utils/src/test/resources/log4j.properties 7 files changed, 58 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12141/4 -- To view, visit http://gerrit.cloudera.org:8080/12141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9752914a426dd1572610f891dad4d4778d04f79c Gerrit-Change-Number: 12141 Gerrit-PatchSet: 4 Gerrit-Owner: Brian McDevitt Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Removed reviewer Kudu Jenkins. -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: helifu
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772 PS6, Line 772: con > I have no idea whether we need to repair this case, because this patch does As we repair when 'the data file exists + size == 0, the metadata file exists + size < min'. I don't see reasons why not treat the case 'the data file exists + size == 0, the metadata file exists + no live blocks' the same. But I am also fine with the current approach. -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 24 Jan 2019 21:57:07 + Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 8: Verified+1 Code-Review+2 (1 comment) Overriding Jenkins, the Python test failures have already been fixed upstream. http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772 PS6, Line 772: con > As we repair when 'the data file exists + size == 0, the metadata file exis Won't this case be handled by the existing "delete dead containers at startup" repair logic? That is, won't the container be deleted? -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 24 Jan 2019 22:00:19 + Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 8: Rebase the patch to pick up the python fix. -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 24 Jan 2019 21:57:51 + Gerrit-HasComments: No
[kudu-CR] [tools] Add table scan tool
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
..
Patch Set 10:
(25 comments)
Thanks for changing the query language! I think it's nicer now. It looks pretty
good. I just had one big structural ask which is to separate all the code for
the scanning into a file separate from tool_action_table.cc.
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG@9
PS10, Line 9: , several
New sentence: ". Several..."
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG@10
PS10, Line 10: the
Remove.
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG@23
PS10, Line 23: All sub-predicates combined together as
: '[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
: operator is supported now.
For clarity, I'd rephrase this as:
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h@314
PS10, Line 314: if (direct_data_.empty()) {
: return KuduRowResult(projection_, nullptr);
: }
Is this change related to the tool? What's its purpose?
It's a semantic change because it doesn't look like the second arg to the
KuduRowResult constructor would ever be nullptr in the original code.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@285
PS10, Line 285: SCOPED_TRACE(*stdout_lines);
: ASSERT_OK(s);
Could you add 'stderr' and 'SCOPED_TRACE(stderr)' back in here? Like how it is
in 'RunActionStderrLines'.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@394
PS10, Line 394: string projections;
: for (const auto& column : columns) {
: projections += (column.second + ",");
: }
For this sort of pattern, it's also possible to accumulate the values in a
vector and then use JoinStrings from kudu/gutil/strings/join.h:
vector acc;
for (const auto& column : columns) {
acc.push_back(column.second);
}
const string projection = JoinStrings(acc, ",");
This way is fine though...up to you if you want to change it.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@400
PS10, Line 400: RunActionStderrString
I think we want to use RunActionStdoutLines here. The tool should print the
rows on stdout, and we should check each line matches the expected format for a
row.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@407
PS10, Line 407: string line_pattern(" \\(");
: int i = 0;
: for (const auto& column : columns) {
: // Check matched rows.
: line_pattern += Substitute("$0 $1=$2",
: column.first, column.second, column.second == "key"
? to_string(value) : ".*");
: if (++i < columns.size()) {
: line_pattern += (", ");
: }
: }
: line_pattern += (")");
It'd be possible to simplify this a bit with JoinStrings-- you could get rid of
'i', for example.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@421
PS10, Line 421: cost
Remove.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2361
PS10, Line 2361: shared_ptr client;
: ASSERT_OK(cluster_->CreateClient(nullptr, ));
Right now you are not using the client.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2365
PS10, Line 2365: &
nit: Use a value instead of reference.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2383
PS10, Line 2383: "[\"AND\",[\"=\",\"key\",1]]"
C++ raw string literals would help make this more readable. See
https://en.cppreference.com/w/cpp/language/string_literal #6. In this case,
"[\"AND\",[\"=\",\"key\",1]]"
could be
R"*(["AND",["=","key",1]])*"
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2404
PS10, Line 2404: // TODO how to match?
You can use the 'client' you created above to insert one more row that has one
column null. The default schema of the TestWorkload is
inline Schema GetSimpleTestSchema() {
return Schema({
[kudu-CR](gh-pages) [blog] a blogpost about location awareness in Kudu
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12119 ) Change subject: [blog] a blogpost about location awareness in Kudu .. Patch Set 4: (18 comments) http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md File _posts/2018-12-20-location-awareness.md: http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@50 PS4, Line 50: the http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@55 PS4, Line 55: location string for the specified IP address/hostname. : The Is this meant to be a paragraph break? http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@61 PS4, Line 61: try to try http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@61 PS4, Line 61: the replica replicas http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@62 PS4, Line 62: if Remove. http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@62 PS4, Line 62: keep to keep http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@64 PS4, Line 64: as well Remove, or remove "Also". http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@65 PS4, Line 65: about to connect to the closest tablet server I'd say "attempt to read from the closest tablet server" since that's all we actually use it for. http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@67 PS4, Line 67: The Kudu leader master assigns location for a client when it connects to a : cluster. The assigned location string is sent back from the leader master : to the client along with other cluster-specific information. This is redundant with the previous paragraph. http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@84 PS4, Line 84: a http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@92 PS4, Line 92: placement policies I prefer to say "placement policy" singular since we only have one. Also I think you ought to define the placement policy here (i.e. restate the "use case" from the beginning as the placement policy). http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@94 PS4, Line 94: The automatic re-replication and the initial placement of new replicas both : attempt to spread replicas across locations so that the failure of tablet : servers in one location does not make tablets unavailable. This is basically the placement policy, so I'd just make it clear that "placement policy" is referring to this condition. http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@101 PS4, Line 101: policies Here and below, like I said, I prefer "policy" since we have just one. http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@139 PS4, Line 139: location each location http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@142 PS4, Line 142: Examples This section is really nice. Thanks for adding it. http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@147 PS4, Line 147: squares nit: rectangles or boxes http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@181 PS4, Line 181: result resulting http://gerrit.cloudera.org:8080/#/c/12119/4/_posts/2018-12-20-location-awareness.md@188 PS4, Line 188: land receive -- To view, visit http://gerrit.cloudera.org:8080/12119 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I10b30a80d5661fb889a11285b8118cdea1a87cd2 Gerrit-Change-Number: 12119 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 24 Jan 2019 18:57:47 + Gerrit-HasComments: Yes
[kudu-CR] [backup] Use upsert on restore
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12268 Change subject: [backup] Use upsert on restore .. [backup] Use upsert on restore Changes the restore job to us upserts instead of inserts so that Spark task retries do not fail when a key already exists. Change-Id: I9905fe4301db8c06fe4f18318ccb04b4179a1601 --- M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/12268/1 -- To view, visit http://gerrit.cloudera.org:8080/12268 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9905fe4301db8c06fe4f18318ccb04b4179a1601 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] generic iterators: basic MergeIterator dominance
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12197 ) Change subject: generic_iterators: basic MergeIterator dominance .. Patch Set 7: Code-Review+2 (6 comments) http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc@102 PS7, Line 102: <> I think you can remove the need for dispose, etc in the implementation by specifying : public list_base_hook>> Have you tried something like that? Per https://www.boost.org/doc/libs/1_69_0/doc/html/intrusive/using_smart_pointers.html I am not 100% sure about this, though, and maybe it's less relevant if we are using disposal lambdas to move elements between lists. http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc@104 PS7, Line 104: NOLINT(*) Just curious: why is NOLINT necessary? How about a "using" alias declaration instead, if that prevents the lint warning? http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc@274 PS7, Line 274: underflowing nit: overflow in the negative direction is still overflow as opposed to insufficient precision http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc@333 PS7, Line 333: domination makes me think of https://www.youtube.com/watch?v=lYPFrXvc2rE=2m34s ?? http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc@459 PS7, Line 459:restart_inner_loop: nit: indentation here appears to be 3 spaces and I assume you meant it to be 0 or 6 spaces? http://gerrit.cloudera.org:8080/#/c/12197/7/src/kudu/common/generic_iterators.cc@608 PS7, Line 608: bounds are lower bound is? -- To view, visit http://gerrit.cloudera.org:8080/12197 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If59d831240af15bfa7ef5709ec3d105d13b28322 Gerrit-Change-Number: 12197 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 24 Jan 2019 08:56:52 + Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Hello Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12239 to look at the new patch set (#7). Change subject: [fs]: delete the (orphaned) metadata file when repairing .. [fs]: delete the (orphaned) metadata file when repairing With KUDU-2636 fixed, we now support deleting dead containers at runtime. Because this involves deleting a pair of files, there's a time window in which it's possible for there to be just one file. A well-timed crash or kill -9 can make this a permanent state that fails the log block manager at startup. Indeed, a loop of dense_node-itest failed 2/1000 times in this way. This patch fixes the problem by detecting "orphaned" metadata files at startup and deleting them. A metadata file is orphaned if it has no corresponding data file and if it contains nothing but dead blocks. Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab --- M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 2 files changed, 349 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/7 -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 ) Change subject: [fs]: delete the (orphaned) metadata file when repairing .. Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@768 PS6, Line 768: // |DATA\METADATA| NONE EXIST | EXIST && < MIN | EXIST && NO LIVE BLOCKS | EXIST && LIVE BLOCKS| > I think it may be better to move this explanation to L546. Done http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772 PS6, Line 772: OK > Not sure why not auto repair this case as well? I have no idea whether we need to repair this case, because this patch does not change the old logic, but only adds a new repaired case where an orphaned metadata file has no live blocks and no corresponding data file either. So, do we need to fix it? @haohao @adar. http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@830 PS6, Line 830: metadata_size > Since based on the current logic, metadata_size is 0 when the metada file d Done http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@840 PS6, Line 840: quick check whether there is no live blocks > nit: quickly check whether or not there is any live blocks. Done http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@867 PS6, Line 867: orphaned metadata file > nit: orphaned metadata file with no live blocks. Done -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 6 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 24 Jan 2019 08:15:02 + Gerrit-HasComments: Yes
