Add timeout hint protocol flag CB-569 #182

brickZA · 2017-04-12T12:44:38Z

Idea is that we want a server to explicitly tell a client that it can use the ?request-timeout-hint request without the client having to guess.

ajoubertza

Implementation looks OK. Where are we going to describe the feature in this repo? E.g. what does a ?request-timeout-hint look like. Will you update the guidelines, or some other doc?

ajoubertza · 2017-04-12T15:46:41Z

katcp/core.py

@@ -596,6 +596,9 @@ class ProtocolFlags(object):

    MULTI_CLIENT = 'M'
    MESSAGE_IDS = 'I'
+    # New proposal flag to indicate that a device supports ?request-timeout-hint
+    # See CB-2051
+    REQUEST_TIMEOUT_HINTS = 'T'


Please add to the docstring, line 572.

Done in pending commit

brickZA · 2017-04-13T05:59:16Z

@ajoubertza At the moment the only actual implementation of ?request-timeout-hint is in proxy_base.py in katcore, where the request handler docstring describes it completely. To define it properly I suggest we try and add it to the KATCP spec, which would provide the definitive definition. Then we could probably also add an optional implementation (with proper docstring) to the top level DeviceServer class (only activated if the correct protocol flags are specified)

ajoubertza · 2017-04-13T06:10:38Z

@brickZA Yes, it is nicely described in katcore, but that isn't a public repo yet. If we add the flag here, we have to describe its purpose, so that it can be used. A optional implementation would be great.

brickZA · 2017-04-13T06:17:43Z

@ajoubertza I agree, but I am willing to postpone that work :) Hence the TODO comment. I also made a backlog item in JIRA. Arm is available for twisting though.

ajoubertza · 2017-04-13T06:21:20Z

I'd like to hear some other opinions. @lvdheever?

brickZA · 2017-04-13T06:55:53Z

Actually, let me just see how much effort it would be to port the katcore implementation to katcp

lvdheever · 2017-04-13T07:15:19Z

Agreed that it would be great if it can be moved to katcp to wrapt this up. The problem with these "TODO"s and related JIRA tickets is that they never get enough priority to get attention. So I am against leaving little bits of TODOs for later. Rather just do it now if it is not a substantial amount of work.

brickZA · 2017-04-13T07:30:24Z

I have come up with a nice clean way of doing it, but I will need some bits from #177

I only need to add some tests to PR177, so I'll go ahead and do that and then we can merge it and go forward.

…flags

…O CB-569

martinslabber · 2017-04-19T00:18:28Z

katcp/core.py

+        if self.request_timeout_hints and not version_supports_hints:
+            raise ValueError(
+                'REQUEST_TIMEOUT_HINTS only suported in katcp v{}.{} and newer'
+                .format(self.major, self.minor))


Please correct reported version info in error & s/suported/supported/.

raise ValueError( 'REQUEST_TIMEOUT_HINTS only supported in katcp v{}.{} and newer' .format(*self.REQUEST_TIMEOUT_HINTS_MIN_VERSION)

Fixed and pushed.

martinslabber · 2017-04-19T00:27:51Z

katcp/core.py

            if name.startswith("request_"):
                request_name = convert_method_name("request_", name)
-                mcs._request_handlers[request_name] = getattr(mcs, name)
-                assert(mcs._request_handlers[request_name].__doc__ is not None)
+                if mcs.check_protocol(handler):


To be more DRY you could move the if mcs.check_protocol(handler): and assert out of the request/inform/reply condition.

yes, but then you need an additional if statement to check if any of the three cases were triggered, etc. I agonised about it for minutes ;)

martinslabber · 2017-04-19T00:29:26Z

katcp/core.py

+        try:
+            protocol_info = mcs.PROTOCOL_INFO
+        except Exception:
+            import pdb ; pdb.set_trace()


Seem you are still busy here.

aah, thanks for catching that :)

It is only activated if the server protocol is set to the (proposed) KATCP v5.1 spec and the timeout hint protocol flag is enabled.

brickZA · 2017-04-19T07:04:55Z

@ajoubertza @martinslabber @lvdheever OK, now ready for re-review. Added default timeout hint request implementation and some machinery for including/excluding request handlers on the basis of the enabled KATCP server protocol flags / protocol version.

ajoubertza

Functionality looks good. Just some comment pedantry to consider.

ajoubertza · 2017-04-19T09:55:55Z

katcp/kattypes.py

+    return decorator
+
+def request_timeout_hint(timeout_hint):
+    """Add recommended client timeout hint to a request for request


Suggest you start the docstring with Decorator;, like the other ones. It makes it more obvious for me.

Added in pending commit.

ajoubertza · 2017-04-19T09:57:22Z

katcp/kattypes.py

+    --------
+    >>> class MyDevice(DeviceServer):
+    ...     @return_reply(Int())
+    ...     @request_timeout_hint


Shouldn't there be a parameter here? E.g. @request_timeout_hint(15.0)

Fixed in pending commit.

ajoubertza · 2017-04-19T09:58:55Z

katcp/kattypes.py

+    Parameters
+    ----------
+    timeout_hint : float (seconds) or None
+        How long the decorated request should reasonably take to reply


Please add some information about what happens if the hint is set to None.

Added in pending commit

ajoubertza · 2017-04-19T11:19:41Z

katcp/test/test_server.py

+    expected_connect_messages = (
+            r'#version-connect katcp-protocol 5.0-IM',
+            # Will have to be updated for every library version bump
+            r'#version-connect katcp-library katcp-python-%s' % katcp_version,


Does this comment still apply, if you're pulling the version from __version__, or are you referring to the 5.0-IM line?

I think you are right, it does not apply. The 5.0 bit should stay the same since that is the specified protocol version, although the library default might change at some stage.

Clarified in pending commit.

ajoubertza · 2017-04-19T11:22:56Z

katcp/test/test_server.py

+        self._assert_msgs_equal(req.inform_msgs,
+                                ['#request-timeout-hint help 0.0'])
+
+        # Now set hint on help command and check that it is reflected Note, in


The "Note" shouldn't be capitalised, or is a full stop missing?

Punctuation updated in pending commit. I appreciate the attention to detail / level of pedantry, whichever way you prefer to look at it :)

ajoubertza · 2017-04-19T11:26:29Z

katcp/test/test_server.py

+        self._test_handler_protocol_filters(self.DeviceWithEverything,
+                                            self.all_expected_handlers)
+
+    def test_handler_protocol_filters_five_with_nothing(self):


Shouldn't this be a five_one, not a five?

Corrected in pending commit.

ajoubertza · 2017-04-19T11:28:57Z

katcp/test/test_server.py

+        """Test handler filtering where protocol flags and version exclude some"""
+
+
+        class DeviceVersionFiveOneWithMultiClient(self.DeviceWithEverything):


Too many blank lines, before this class?

Made whitespace consistent between these related test methods.

ajoubertza · 2017-04-19T11:30:36Z

katcp/test/test_server.py

+        self.assertEqual(set(), actual_device_handlers & not_expected_handlers)
+
+
+    def test_handler_protocol_filters_all(self):


Too many blank lines before this method?

Fixed in pending commit.

brickZA · 2017-04-19T11:51:58Z

@ajoubertza OK, updates pushed and ready for renewed scrutiny :)

ajoubertza

All good!

Add timeout hint protocol flag CB-569

9b71c7b

ajoubertza reviewed Apr 13, 2017

View reviewed changes

Add ProtocolFlags docstring for 'T' flag

85bcee3

brickZA added 2 commits April 18, 2017 09:19

Merge branch 'master' into user/nmarais/CB-569/timeout-hint-protocol-…

f965c61

…flags

Filter included handlers optional on the basis of server PROTOCOL_INF…

8e06a7a

…O CB-569

martinslabber reviewed Apr 19, 2017

View reviewed changes

brickZA added 2 commits April 19, 2017 09:00

Add default request-timeout-hint implementation to server.py

643564d

It is only activated if the server protocol is set to the (proposed) KATCP v5.1 spec and the timeout hint protocol flag is enabled.

Remove debugging code accidentally commited

d70b43b

Fix exception error string as per PR182 comment.

0a4317d

ajoubertza suggested changes Apr 19, 2017

View reviewed changes

Style / documentation improvements are per PR182

7194182

ajoubertza approved these changes Apr 19, 2017

View reviewed changes

brickZA merged commit 9750769 into master Apr 19, 2017

brickZA deleted the user/nmarais/CB-569/timeout-hint-protocol-flags branch April 19, 2017 12:26

brickZA mentioned this pull request Apr 19, 2017

Potential DRY version of device metaclass #184

Closed

		"""Test handler filtering where protocol flags and version exclude some"""


		class DeviceVersionFiveOneWithMultiClient(self.DeviceWithEverything):

		self.assertEqual(set(), actual_device_handlers & not_expected_handlers)


		def test_handler_protocol_filters_all(self):

Add timeout hint protocol flag CB-569 #182

Add timeout hint protocol flag CB-569 #182

Conversation

brickZA commented Apr 12, 2017

ajoubertza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brickZA commented Apr 13, 2017

ajoubertza commented Apr 13, 2017

brickZA commented Apr 13, 2017 • edited Loading

ajoubertza commented Apr 13, 2017

brickZA commented Apr 13, 2017

lvdheever commented Apr 13, 2017

brickZA commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brickZA commented Apr 19, 2017

ajoubertza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajoubertza Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brickZA commented Apr 19, 2017

ajoubertza left a comment

Choose a reason for hiding this comment

brickZA commented Apr 13, 2017 •

edited

Loading

ajoubertza Apr 19, 2017 •

edited

Loading