> On June 2, 2015, 2:59 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/resources/common-services/STORM/0.9.1.2.1/package/scripts/params_linux.py, > > lines 107-110 > > <https://reviews.apache.org/r/34945/diff/1/?file=976677#file976677line107> > > > > A couple issues with this: > > > > 1) If someone changes the default(...,None) to default(...,"") in the > > future, then this check will fail to remove the config > > > > 2) This only removes the config from the dictionary if it exists. If > > we're deleting the entry, should we even have parameters for it? Shouldn't > > we also remove default_topology_max_replication_wait_time_sec and > > default_topology_min_replication_count > > Dmytro Shkvyra wrote: > 1) I dont see situation if somebody will change safe python type None to > "". Do you know this situation? > 2) I remove .default properties because they are should not be in final > config (To be honest they do not spoil anything), > default_topology_max_replication_wait_time_sec and > default_topology_min_replication_count do not use in other scripts. They just > need to calculate real values for > actual_topology_max_replication_wait_time_sec and > actual_topology_min_replication_count which will be inserted while config > generation > > Jonathan Hurley wrote: > 1) It's actually a common pattern I've seen in our params.py files; many > times we use "" instead of None. This change, although simple on the surface, > would cause your removal code to break. Would it not be better to just do a > `if "topology.max.replication.wait.time.sec.default" in > config['configurations']['storm-site']` instead of relying on a variable. > > 2) I see, so these defaults are used to set the actuals, and then > discarded. OK, so I'm fine with leaving them in.
1) I have used variables because they are already defined. Ok. If it will affect something I will use "topology.max.replication.wait.time.sec.default" in config['configurations']['storm-site'] - Dmytro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34945/#review86221 ----------------------------------------------------------- On June 2, 2015, 2:50 p.m., Dmitro Lisnichenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34945/ > ----------------------------------------------------------- > > (Updated June 2, 2015, 2:50 p.m.) > > > Review request for Ambari, Jonathan Hurley, Sumit Mohanty, and Vitalyi > Brodetskyi. > > > Bugs: AMBARI-11611 > https://issues.apache.org/jira/browse/AMBARI-11611 > > > Repository: ambari > > > Description > ------- > > Now Ambari always use 1 as the value. This will render Storm-HA to not work > out of the box. We want to find the formula. > So these 2 entries should be deleted. > topology.max.replication.wait.time.sec.default : 60 > topology.min.replication.count.default : 1 > > Fix failing unit test > > > Diffs > ----- > > > ambari-server/src/main/resources/common-services/STORM/0.9.1.2.1/package/scripts/params_linux.py > 6c3078b > > Diff: https://reviews.apache.org/r/34945/diff/ > > > Testing > ------- > > [INFO] Rat check: Summary of files. Unapproved: 0 unknown: 0 generated: 0 > approved: 136 licence. > [INFO] > ------------------------------------------------------------------------ > [INFO] Reactor Summary: > [INFO] > [INFO] Ambari Views ...................................... SUCCESS [1.718s] > [INFO] Ambari Metrics Common ............................. SUCCESS [0.748s] > [INFO] Ambari Server ..................................... SUCCESS [49.324s] > [INFO] Ambari Agent ...................................... SUCCESS [18.685s] > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 1:11.292s > [INFO] Finished at: Tue Jun 02 17:49:39 EEST 2015 > [INFO] Final Memory: 48M/482M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Dmitro Lisnichenko > >
