> On June 15, 2016, 11:44 p.m., Andrew Onischuk wrote:
> > ambari-server/src/main/resources/common-services/HIVE/,
> >  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?

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:
        Logger.warning('mysql process not running')

but I think it will be reduntant with the exception.

What do you think?

- Juanjo

This is an automatically generated e-mail. To reply, visit:

On June 15, 2016, 10:39 p.m., Juanjo  Marron wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48766/
> -----------------------------------------------------------
> (Updated June 15, 2016, 10:39 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/
>  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