This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/549/ |
On July 12th, 2013, 11:33 a.m. UTC, Jan Safranek wrote:
cli-tools/lmi/lmi_client_shell.py (Diff revision 2) 751 unique_str = ''.join(random.choice(string.ascii_uppercase + string.digits) for x in range(6))There is risk of clashing with other listeners. Will add_handler() throw an error in this case?
Very low chance to clash. Currently the handler callback will be overwritten. Ok, maybe I could add some checks.
On July 12th, 2013, 11:33 a.m. UTC, Jan Safranek wrote:
cli-tools/lmi/lmi_client_shell.py (Diff revision 2) 753 destination_port = 10240This port should be probably configurable. (maybe in next releases)
OK.
On July 12th, 2013, 11:33 a.m. UTC, Jan Safranek wrote:
cli-tools/lmi/lmi_client_shell.py (Diff revision 2) 764 filter_name = "LMI:%s:Changed" % job_inst.classnameExpected name of the indication filter must be documented somewhere. BTW, there is no lmishell documentation so far.
The indication filter name comes from (eg.) your provider. But I can add some comment, or something, how the filters' names should look like.
On July 12th, 2013, 11:33 a.m. UTC, Jan Safranek wrote:
cli-tools/lmi/lmi_client_shell.py (Diff revision 2) 840 cond.wait(15)There is a lot of magic numbers in this file, '15' is just an example. Please use constants with meaningful names.
On purspose, I was expecting an opinion, if it is a suitable constant. Will be switched to a constant, of course.
On July 12th, 2013, 11:33 a.m. UTC, Jan Safranek wrote:
cli-tools/lmi/lmi_client_shell.py (Diff revision 2) 852 if _LmiInstanceMethod.is_job_finished(job_inst):853 job_out_params.value = job_inst.JobOutParameters854 listener.stop()855 break
If I understand it correctly, this checks every minute that the job instance is still in progress by calling GetInstance(). The GetInstance() is useful only to check that the job had not finished before we subscribed for job indications. Therefore the lmishell should check much earlier than one minute, probably in 0.1-0.5 seconds, and check only once.
There is a check before the "waiting loop". If a job has already finished before entering the loop, the loop body will not be executed.
On July 12th, 2013, 11:33 a.m. UTC, Jan Safranek wrote:
cli-tools/lmi/lmi_client_shell.py (Diff revision 2) def __call__(self, *args, **kwargs):905 filter_classnames = (906 "LMI_StorageJob",907 "LMI_SoftwareInstallationJob",908 "LMI_NetworkJob"
909 )
This list should be configurable in later versions. Or you can check for CIM_ConcreteJob subclasses (if it is simple).
I would stick with a configuration.
On July 12th, 2013, 11:33 a.m. UTC, Jan Safranek wrote:
cli-tools/lmi/lmi_client_shell.py (Diff revision 2) def __call__(self, *args, **kwargs):929 sleep_time = 1930 while not _LmiInstanceMethod.is_job_finished(job_inst):931 # Sleep, a bit longer in every iteration
932 time.sleep(sleep_time)933 if sleep_time < 128:934 sleep_time *= 2935 (refreshed, _, errorstr) = job_inst.refresh()936 if not refreshed:937 if LmiBaseClient._get_use_exceptions():938 raise LmiSynchroMethodCallError(errorstr)939 return LmiReturnValue(940 rval = -1,941 errorstr = errorstr,942 hostname = self._client.hostname,943 use_display_hook = True)944 call_rparams = self.__transform_return_values({"out" : job_inst.JobOutParameters})Cut it to separate method?
OK.
- Peter
On July 12th, 2013, 11:01 a.m. UTC, Peter Hatina wrote:
Review request for OpenLMI Developers.
By Peter Hatina.
Updated July 12, 2013, 11:01 a.m.
Repository:
openlmi-tools
Description
Testing
Diffs
|