Mailing-List: contact dev-help@ambari.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@ambari.apache.org
Content-Type: multipart/alternative;
 boundary="===============0330257353491249177=="
MIME-Version: 1.0
Subject: Re: Review Request 34945: Delete incorrect Storm-HA configs. Fix unit
 tests
From: "Jonathan Hurley" <jhurley@hortonworks.com>
To: "Sumit Mohanty" <smohanty@hortonworks.com>,
 "Vitalyi Brodetskyi" <vbrodetskyi@hortonworks.com>,
 "Jonathan Hurley" <jhurley@hortonworks.com>
Cc: "Dmitro Lisnichenko" <dlysnichenko@hortonworks.com>,
 "Dmytro Shkvyra" <dshkvyra@hortonworks.com>, "Ambari" <dev@ambari.apache.org>
Date: Tue, 02 Jun 2015 15:53:58 -0000
Message-ID: <20150602155358.1498.39098@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Jonathan Hurley" <noreply@reviews.apache.org>
References: <20150602145917.1498.40242@reviews.apache.org>
In-Reply-To: <20150602145917.1498.40242@reviews.apache.org>
Reply-To: "Jonathan Hurley" <jhurley@hortonworks.com>

--===============0330257353491249177==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> 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
> 
>


--===============0330257353491249177==--