> On June 15, 2016, 11:44 p.m., Andrew Onischuk wrote:
> > ambari-server/src/main/resources/common-services/HIVE/0.12.0.2.0/package/scripts/mysql_service.py,
> >  line 54
> > <https://reviews.apache.org/r/48766/diff/1/?file=1420515#file1420515line54>
> >
> >     We should probably do a checked_call here to give an exception with a 
> > good information in case we cannot do this.
> >     
> >     Also why to we need timeout here?
> 
> Juanjo  Marron wrote:
>     Hi Andrew!
>     Thanks for the quick review
>     
>     I will remove the timeout. I saw several ps -ef calls fixing the timeout 
> and I thought it could be appropiate.
>     
>     Regarding the check call. I dont know if I followed you , but:
>     If the shell call  fails in the mysqladmin command, mysql_pid_file will 
> be None (meaning mysql service is not running) and check_process_status() 
> will throw ComponentIsNotRunning exception as expected
>     I could capture a log message:
>     else:
>             Logger.warning('mysql process not running')
>     
>     but I think it will be reduntant with the exception.
>     
>     What do you think?
> 
> Juanjo  Marron wrote:
>     Ohh I see, I guess you are refering to shell.checked_call() for testing. 
> I was not aware of this resource. (Always learning, sorry)
>     Any good eaxmple I could follow?
>     
>     Thanks
> 
> Andrew Onischuk wrote:
>     Can we do something like this:
>     
>     
>         def get_mysql_pid_file():
>           cmd = 'mysqladmin variables'
>           code, output = shell.checked_call(cmd)
>       
>           for line in output.splitlines():
>             if (line.find("pid_file") != -1):
>               mysql_pid_file  = line.split()[3].strip()
>               break
>           else:
>             raise Fail(format("Output of '{cmd}' didn't contain pid_file. 
> Output: {output}"))  
>           return mysql_pid_file 
>       
>       
>     Also please make sure 'mysqladmin variables' is accessible on 
> Suse/Ubuntu/Centos.

Added new diff following Andrew intructions

Thanks for the comments


- Juanjo


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


On June 16, 2016, 9:20 p.m., Juanjo  Marron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48766/
> -----------------------------------------------------------
> 
> (Updated June 16, 2016, 9:20 p.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, and Dmitro 
> Lisnichenko.
> 
> 
> Bugs: AMBARI-13238
>     https://issues.apache.org/jira/browse/AMBARI-13238
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> The MySQL service status in mysql_service.py simply checks for a process with 
> name mysqld. In our environment, a different service ran another MySQL 
> instance on that node and as a result, the status of the MySQL service in 
> Hive showed green (because it could find a mysqld process) even though the 
> instance used by Hive wasn't started. That also made the "Start Service" 
> action for Hive fail, because the metastore service couldn't connect to the 
> MySQL database.
> 
> The proposed fix makes the service check more robust by retrieving the 
> pid_file of the MySQL instance first by running "mysqladmin variables" and 
> parsing out the pid_file. Then it checks if the process exists. 
> 
> A new pacth is proposed based on the reviews added to the original one 
> (https://reviews.apache.org/r/39146/)
> 
> The patch relies now on the pid (similar to other status check). mysql 
> pid_file is not known a priori (it is not a service property similar to other 
> ambari components) and it depends on the OS. 
> The pid file location is obatined with  "mysqladmin variables" command (which 
> seems recognized by all the Ambari supported OS). 
> The Hive MySQL server instance is started without passing in any parameters 
> so It uses the default configuration. Any other MySQL instance that could be 
> running on the machine would need to be started with explicit config.
> Hence just running mysqladmin will return the values for the Hive MySQL 
> instance.
> The pid dile name is parsed by a new method which has been created to obatin 
> it from the mysqladmin variables return.
> The status relies now on check_process_status() wich is already an ambari 
> function.
> Unit Test have been modified accordingly to avoid the pgrep verification that 
> could create conflicts with others mysql instances working on the same node.
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/resources/common-services/HIVE/0.12.0.2.0/package/scripts/mysql_service.py
>  7862774 
>   ambari-server/src/test/python/stacks/2.0.6/HIVE/test_mysql_server.py 
> 827f6f7 
> 
> Diff: https://reviews.apache.org/r/48766/diff/
> 
> 
> Testing
> -------
> 
> +1 overall. Here are the results of testing the latest attachment 
> http://issues.apache.org/jira/secure/attachment/12810584/AMBARI-13238.patch
> against trunk revision .
> +1 @author. The patch does not contain any @author tags.
> +1 tests included. The patch appears to include 1 new or modified test files.
> +1 javac. The applied patch does not increase the total number of javac 
> compiler warnings.
> +1 release audit. The applied patch does not increase the total number of 
> release audit warnings.
> +1 core tests. The patch passed unit tests in ambari-server.
> Test results: 
> https://builds.apache.org/job/Ambari-trunk-test-patch/7367//testReport/
> Console output: 
> https://builds.apache.org/job/Ambari-trunk-test-patch/7367//console
> This message is automatically generated.
> 
> 
> Thanks,
> 
> Juanjo  Marron
> 
>

Reply via email to