> On Aug. 3, 2016, 12:33 a.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/resources/common-services/HIVE/0.12.0.2.0/package/scripts/mysql_service.py,
> >  line 36
> > <https://reviews.apache.org/r/48766/diff/2/?file=1421794#file1421794line36>
> >
> >     What happens if pid_file is None?

In case pid_file is not in the mysqladmin variables output, mysql_pid_file will 
remain None (as initialized) and the Failure ("Output of '{0}' didn't contain 
pid_file. Output: '{1}'").format(cmd, output)  will be raised

Also, if  check_process_status(pid_file) is called with None the method defined 
on resource_management/libraries/functions/check_process_status.py will log the 
info and raise ComponentIsNotRunning() exception

I think we are fine if pid_file is None


> On Aug. 3, 2016, 12:33 a.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/resources/common-services/HIVE/0.12.0.2.0/package/scripts/mysql_service.py,
> >  line 59
> > <https://reviews.apache.org/r/48766/diff/2/?file=1421794#file1421794line59>
> >
> >     This needs to check that output is not None

I added this output verification on the new patch submitted


> On Aug. 3, 2016, 12:33 a.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/resources/common-services/HIVE/0.12.0.2.0/package/scripts/mysql_service.py,
> >  line 60
> > <https://reviews.apache.org/r/48766/diff/2/?file=1421794#file1421794line60>
> >
> >     Remove parentheses and check >= 0
> >     Further, before accessing index 3, should check the length.
> >     
> >     To make this a bit more robust, please put a try-catch around it, but 
> > still raise Fail if code is non-zero.

I changed the if condition to check >= 0 and validate the list lenght before 
splitting it.

Regarding the no-zero code, we should not forget we are checking the service 
status, so MySQL can be started or stopped at this time   
When mysql is stopped, mysqladmin variables will return error code (!=0) and 
the following output:

Output of 'mysqladmin variables' didn't contain pid_file. Output: mysqladmin: 
connect to server at 'localhost' failed
error: 'Can't connect to local MySQL server through socket 
'/var/lib/mysql/mysql.sock' (2)'
Check that mysqld is running and that the socket: '/var/lib/mysql/mysql.sock' 
exists!

And I believe it shouldnt be considered as an error or catched as an excpetion, 
it just means MySQL is in a stopped state.
pid_file will be None and check_process_status will handle it as 
ComponentIsNotRunning


- Juanjo


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


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