> On June 27, 2017, 6:45 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/utilities/KerberosIdentityCleaner.java > > Lines 72 (patched) > > <https://reviews.apache.org/r/60431/diff/5/?file=1763803#file1763803line72> > > > > Should probably be named `getComponentDescriptor`
Is this an Ambari convention? Under what circumstances do we like using the get prefix? I feel most of the times this doesn't add too much extra information. Yes, this method returns a value, but most of the methods return a value anyway (there are programming languages where every function returns a value), and I would rather highlight the differences instead of the similarities. Some people like using the get prefix when the method returns a stored value instead of a calculated one. This is also debatable IMO, but here it's not the case, because these methods do calculations. And also they're private with very little scope. > On June 27, 2017, 6:45 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/utilities/KerberosIdentityCleaner.java > > Lines 77 (patched) > > <https://reviews.apache.org/r/60431/diff/5/?file=1763803#file1763803line77> > > > > Should be named `getIdentityNames` See my comment above. > On June 27, 2017, 6:45 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/utilities/KerberosIdentityCleaner.java > > Lines 96 (patched) > > <https://reviews.apache.org/r/60431/diff/5/?file=1763803#file1763803line96> > > > > Should be named `getActiveIdentities` See my comment above. > On June 27, 2017, 6:45 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/utilities/KerberosIdentityCleaner.java > > Lines 114 (patched) > > <https://reviews.apache.org/r/60431/diff/5/?file=1763803#file1763803line114> > > > > Should be named `isSameComponent` See my comment above. - Attila ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60431/#review179008 ----------------------------------------------------------- On June 28, 2017, 8:06 a.m., Attila Magyar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60431/ > ----------------------------------------------------------- > > (Updated June 28, 2017, 8:06 a.m.) > > > Review request for Ambari, Balázs Bence Sári, Jaimin Jetly, Laszlo Puskas, > Robert Levas, and Sebastian Toader. > > > Bugs: AMBARI-21343 > https://issues.apache.org/jira/browse/AMBARI-21343 > > > Repository: ambari > > > Description > ------- > > Upon removing a component from a host, the relevant Kerberos identities > should be removed as well. This includes any principals and keytab files. > Care must be taken not to remove any principals or keytab files that are > still in use in the cluster. > > > Entry point is KerberosIdentityCleaner>>componentRemoved. It removes all of > the identities of the uninstalled component, except the ones that are still > used by other services/components. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > e8c986b > > ambari-server/src/main/java/org/apache/ambari/server/controller/DeleteIdentityHandler.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java > d000571 > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java > e4b70fd > > ambari-server/src/main/java/org/apache/ambari/server/controller/OrderedRequestStageContainer.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/utilities/KerberosIdentityCleaner.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractPrepareKerberosServerAction.java > 7824019 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/Component.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/FinalizeKerberosServerAction.java > 0b845d9 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java > 9755bd6 > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java > 2112fcc > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosComponentDescriptor.java > ca9f013 > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java > 86a5e01 > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosIdentityDescriptor.java > a606954 > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptor.java > 0f14ca6 > > ambari-server/src/test/java/org/apache/ambari/server/controller/utilities/KerberosIdentityCleanerTest.java > PRE-CREATION > ambari-web/app/controllers/main/service/item.js 09c7a9c > > > Diff: https://reviews.apache.org/r/60431/diff/6/ > > > Testing > ------- > > Added new unittests > End to end tested manually: > > before all: > - created a cluster with oozie > - enabled kerberos > 1. > - removed oozie > - checked if oozie_server principal and keytab was removed, and other > identites weren't touched > 2. > - made the kdc admin credentials expired > - removed oozie > - supplied kdc admin credentials on the ui > - checked if oozie_server principal and keytab was removed > 3. > - added oozie_server principal to a kerberos.json of an other service (so > that the principal was shared) > - removed oozie > - checked if the oozie_server principal and keytab was NOT removed > 4. > - disabled kerberos > - removed oozie > - checked that there was no identity deletion attempt > > > Existing Tests: > Results : > Tests run: 4790, Failures: 0, Errors: 0, Skipped: 35 > Ran 244 tests in 7.122s > > OK > ---------------------------------------------------------------------- > Total run:1146 > Total errors:0 > Total failures:0 > OK > > > Thanks, > > Attila Magyar > >
