Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20050 )
Change subject: KUDU-3448 Add support for encrypting existing keys ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/20050/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20050/5//COMMIT_MSG@24 PS5, Line 24: they nit: the http://gerrit.cloudera.org:8080/#/c/20050/5//COMMIT_MSG@27 PS5, Line 27: has been deleted Consider introducing a dedicated method there, instead of re-using AddCertAuthorityEntry() from different semantic contexts. http://gerrit.cloudera.org:8080/#/c/20050/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/20050/5/src/kudu/master/catalog_manager.cc@1351 PS5, Line 1351: RETURN_NOT_OK_PREPEND(sys_catalog_->AddCertAuthorityEntry(info), : "IPKI certificate couldn't be written to syscatalog"); Did you consider doing this not in LoadCertAuthorityInfo(), but in the outer scope? For example, in InitCertAuthority(). That way it would be cleaner from the logical perspective: LoadCertAuthorityInfo() would only try to load the data, but not modify it. In that approach, LoadCertAuthorityInfo() could return an extra piece of information (via an 'out' parameter?), so the follow-up actions up updating the CA information would be contained in the InitCertAuthority() scope. http://gerrit.cloudera.org:8080/#/c/20050/5/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/20050/5/src/kudu/master/sys_catalog.cc@927 PS5, Line 927: enc.Add(RowOperationsPB::UPSERT, row); Maybe, introduce a separate dedicated method UpdateCertAuthorityEntry() or RewriteCertAuthorityEntry() for this use-case of encrypting the CA data? Code-wise, it could share the code, but it's important to call a proper method from particular semantic context. The reason is documented in a long comment in CatalogManager::InitCertAuthority() under the 'if (s.IsNotFound())' scope. Basically, that's to have a guarantee that upon first generation of the CA information a multi-master cluster will not end up having two different CA data in the system catalog. -- To view, visit http://gerrit.cloudera.org:8080/20050 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide6ec4fb86325897f2b011aee9643d276044279d Gerrit-Change-Number: 20050 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 15 Jun 2023 22:17:32 +0000 Gerrit-HasComments: Yes
