> On Nov. 29, 2014, 3:32 p.m., Rohit Yadav wrote: > > core/src/com/cloud/agent/api/routing/SetFirewallRulesCommand.java, line 50 > > <https://reviews.apache.org/r/28535/diff/1/?file=778385#file778385line50> > > > > If you want insertion ordered iteration or array (toArrary output) why > > not use LinkedHashSet? Even if TreeSet will return sorted/ordered traversal > > or array, it will add log(n) complexity to already O(n) loop. Should we use > > that and fix any tests accordingly? Can you also share the test and log > > which broke for you? > > ChunFeng Tian wrote: > Rohit Yadav: > > Actually , LinkedHashSet is my first choice too , but can not pass the > junit test . > In method "generateFwRules()" , the toAdd Set only used generate toArray > result which is expected sequenced as input in test code. > > > I can pass test on windows with HashSet but failed on Mac OSX. > > > failed test log (HashSet): > ----------------------------- > > Results : > > Failed tests: > testFirewallRulesCommand(com.cloud.agent.resource.virtualnetwork.VirtualRoutingResourceTest): > > expected:<...1.2/24:,64.10.10.10:[reverted:0:0:0:,64.10.10.10:TCP:22:80:10.10.1.1/24-10.10.1.2/24]:,> > but > was:<...1.2/24:,64.10.10.10:[TCP:22:80:10.10.1.1/24-10.10.1.2/24:,64.10.10.10:reverted:0:0:0]:,> > > testAggregationCommands(com.cloud.agent.resource.virtualnetwork.VirtualRoutingResourceTest): > > expected:<...1.2/24:,64.10.10.10:[reverted:0:0:0:,64.10.10.10:TCP:22:80:10.10.1.1/24-10.10.1.2/24]:,(..) > > Tests run: 139, Failures: 2, Errors: 0, Skipped: 0 > > end > ------------ > > Rohit Yadav wrote: > Hi, I played the patch using LinkedHashSet, it builds fine and the tests > passes for me on OSX using JDK 1.7. In fact the tests have been passing for > most of us in OSX, since this test has been in core module for several months. > > Are you using JDK 7? If you're using JDK 8, some tests and build failures > may happen. > > ChunFeng Tian wrote: > Rohit Yadav, > > You get me out , I check my default osx java version is : java version > "1.8.0_05" . > > When I view the pom.xml under the dir of cloudstack , it required java > 1.7 > > <cs.jdk.version>1.7</cs.jdk.version> > > but , it seemed that the above line can not work . > > > I will close this review request , > > Thanks again . > > ChunFeng
Glad I can help, just install and use JDK 1.7 for now. Feel free to send us more patches and consider to send us Github Pull requests on https://github.com/apache/cloudstack. Thanks. - Rohit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28535/#review63285 ----------------------------------------------------------- On Nov. 29, 2014, 9:22 a.m., ChunFeng Tian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28535/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2014, 9:22 a.m.) > > > Review request for cloudstack. > > > Repository: cloudstack-git > > > Description > ------- > > run test on core failed : Change firewall rule set from HashSet to TreeSet > because HashSet elements are not ordered , which will cause test failed when > elements toArray are not same between windows and mac . > > > Diffs > ----- > > core/src/com/cloud/agent/api/routing/SetFirewallRulesCommand.java be85887 > > Diff: https://reviews.apache.org/r/28535/diff/ > > > Testing > ------- > > after ensure the firewall rule set in TreeSet , toArray will the same order > accross windows and mac platform, and passed test. > > > Thanks, > > ChunFeng Tian > >
