ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Hurley" <jhur...@hortonworks.com>
Subject Re: Review Request 34945: Delete incorrect Storm-HA configs. Fix unit tests
Date Tue, 02 Jun 2015 15:53:58 GMT


> On June 2, 2015, 10:59 a.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

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.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34945/#review86221
-----------------------------------------------------------


On June 2, 2015, 10:50 a.m., Dmitro Lisnichenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34945/
> -----------------------------------------------------------
> 
> (Updated June 2, 2015, 10:50 a.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message