> On Feb. 13, 2017, 10:49 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/PermissionEntity.java, > > lines 243-251 > > <https://reviews.apache.org/r/56599/diff/1/?file=1631928#file1631928line243> > > > > Any reason you didn't use a Set here (especially since the contract > > only defines a Collection)? > > Attila Magyar wrote: > The existing code uses ArrayList. I'm not sure if it will break anything > if I change it to Set. > > Attila Magyar wrote: > @jhurley > > If we change it to set then we should remove the setter, and use > addAuthorization() all over the place. > > public void setAuthorizations(Collection<RoleAuthorizationEntity> > authorizations) { > this.authorizations = authorizations; > } > > > The setAuthorizations() is used at one place in production code, and in > 10 tests. But it will behave differently because it won't preserver the > ordering. Or we can use LinkedHashSet to preserve the ordering too. What do > you think?
A LinkedHashSet will retain order - beyond that, since you're doing the contains() check here which hash to iterate over the whole list, it seems to make sense to use a Set. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56599/#review165341 ----------------------------------------------------------- On Feb. 13, 2017, 8:48 a.m., Attila Magyar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56599/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2017, 8:48 a.m.) > > > Review request for Ambari, Jonathan Hurley, Laszlo Puskas, Nate Cole, Robert > Levas, and Sebastian Toader. > > > Bugs: AMBARI-19984 > https://issues.apache.org/jira/browse/AMBARI-19984 > > > Repository: ambari > > > Description > ------- > > Running ambari-upgrade multiple times cause an exception due to violation of > unique database constraint (the same RoleAuthorization can be added multiple > times to admin permissions). > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/PermissionEntity.java > fb01cca > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/AbstractUpgradeCatalog.java > 4b33bcd > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog242Test.java > 0cd4f12f > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog250Test.java > 1c742ef > > Diff: https://reviews.apache.org/r/56599/diff/ > > > Testing > ------- > > Added new unittest that checks if adding RoleAuthorization is idempotent. > > Tested manually: > - installed ambari 2.4 > - upgraded to ambari 2.5 > - set version back to 2.4 by running: update metainfo set metainfo_value = > '2.4.0' where metainfo_key = 'version'; > - upgraded to ambari 2.5 again > - started ambari-server checked the UI > > > Existing tests ran clealy > > ---------------------------------------------------------------------- > Ran 465 tests in 15.977s > > Tests run: 4915, Failures: 0, Errors: 0, Skipped: 39 > > > Thanks, > > Attila Magyar > >
