Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13013 )
Change subject: WIP master: use AuthzProvider to generate authz tokens ...................................................................... Patch Set 4: (3 comments) I took a quick look, will take another look tomorrow morning. Overall looks good, I just want to get a better understanding of the coverage that's in the new test. http://gerrit.cloudera.org:8080/#/c/13013/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13013/4/src/kudu/master/catalog_manager.cc@2755 PS4, Line 2755: Should we send back : // an error that the client can retry, e.g. if Sentry was down? > Looking through some DDL authz code, seems like if Sentry isn't available, Yep, Hao and I discussed that a bit in the context of https://gerrit.cloudera.org/c/12877/1/src/kudu/integration-tests/master-stress-test.cc#118 comment. I'm curious what's Hao stance on this. http://gerrit.cloudera.org:8080/#/c/13013/4/src/kudu/master/catalog_manager.cc@2762 PS4, Line 2762: RETURN_NOT_OK(token_signer->GenerateAuthzToken( > Stepping through the code, most of it is shuttling strings and ints around, Kudu signs tokens using SHA256 digest. It's pretty fast, you can try to run 'openssl speed sha256' and see your numbers. In my case (2.2 GHz Intel Core i7, 4 cores, 256KB L2 cache per core), assuming our tokens are of size 256 bytes, gives a range roughly 2M tokens per CPU core signed per second. So, my machine would be able to sign about 8M tokens (of size 256 bytes) per second. I think that's not bad for a macBook laptop. I'm not sure it makes sense to cache those instead of creating new ones: it seems to be cheap. aserbin-MBP:master[java-dis-test-logs]$ openssl speed sha256 Doing sha256 for 3s on 16 size blocks: 13543954 sha256's in 2.99s Doing sha256 for 3s on 64 size blocks: 7386135 sha256's in 2.96s Doing sha256 for 3s on 256 size blocks: 3480055 sha256's in 2.99s Doing sha256 for 3s on 1024 size blocks: 1059385 sha256's in 2.99s Doing sha256 for 3s on 8192 size blocks: 137842 sha256's in 2.97s OpenSSL 1.0.2r 26 Feb 2019 built on: reproducible build, date unspecified options:bn(64,64) rc4(ptr,int) des(idx,cisc,16,int) aes(partial) idea(int) blowfish(idx) compiler: /usr/bin/clang -I. -I.. -I../include -fPIC -fno-common -DOPENSSL_PIC -DZLIB -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -arch x86_64 -O3 -DL_ENDIAN -Wall -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM -DECP_NISTZ256_ASM The 'numbers' are in 1000s of bytes per second processed. type 16 bytes 64 bytes 256 bytes 1024 bytes 8192 bytes sha256 72476.01k 159700.22k 297957.89k 362812.79k 380202.58k http://gerrit.cloudera.org:8080/#/c/13013/4/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/13013/4/src/kudu/util/random_util.h@59 PS4, Line 59: c.size() - min_to_return nit: maybe, just have (c.size() + 1 - min_to_return) here and remove the 'if (min_to_return == c.size())' short-circuit above? That way you don't need to think about the difference in the contract of this method and ReservoirSample that might be induced by the short-circuiting above. -- To view, visit http://gerrit.cloudera.org:8080/13013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5404d6437699bc6c7c8bb0e530b202109e8f166 Gerrit-Change-Number: 13013 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 15 Apr 2019 07:53:58 +0000 Gerrit-HasComments: Yes
