> 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?
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 - Juanjo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48766/#review137885 ----------------------------------------------------------- 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/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 > >
