Task #3455
closed
Implement ControlResponse
Added by Alex Afanasyev over 8 years ago. Updated about 8 years ago.
100%
Description
ControlResponse is a necessary element to implement MockFace (in ndn-cxx it is called DummyClientFace). In order to avoid dependency explosion, this class needs to be moved into jndn. It is also general-enough element to be included in the library.
Implementation that should be moved:
Corresponding test case:
Updated by Jeff Burke over 8 years ago
- Assignee changed from Jeff Burke to Anonymous
Ok w/me, assigning to JeffT for review / inclusion.
Updated by Anonymous over 8 years ago
ControlResponse is defined as:
ControlResponse ::= CONTROL-RESPONSE-TYPE TLV-LENGTH
StatusCode
StatusText
<body>?
where the body is not necessarily restricted to a sequence of ControlParameters. In practice, does NFD only use ControlParameters in the the body? If not, does the ControlResponse API need more flexibility?
Updated by Alex Afanasyev over 8 years ago
NFD only uses ControlParameters. However, the ControlReponse is a general structure that (in long-term perspective) can be used in different environments with different types of arguments to the command (e.g., repo-ng may introduce different parameters).
My suggestion would be:
- make API of
ControlReponsegeneral like it is in ndn-cxx (= getBody() returns just whatever memory blob) - have a helper to decode body of control command to
ControlParameters(unless it is too trivial)
I would also want to suggest moving the existing ControlParameters into NFD (net.named_data.jndn.nfd) package, given it is really specific to NFD.
Updated by Anonymous over 8 years ago
We could make getBody return a Blob, but also add getControlParametersList which returns a ControlParameters list exactly like getBody does now. (This would return null if the ControlResponse didn't contain a list of ControlParameters.) Likewise we would add setControlParametersList which does what setBody does now. This solution covers all the currently known use cases and is easier on the developer.
Updated by Alex Afanasyev over 8 years ago
You can drop "List". In NFD, there is just one ControlParameters TLV in the body.
I could be ok with bundling, but prefer still separation of general data structure and NFD specifics. For example, jndn.ControlParamers as general structure and jndn.nfd.ControlParamerters inherited from jndn.ContorlParameters and adding the helpers you mentioned.
Updated by Anonymous over 8 years ago
I'd prefer to avoid over-complicating the class hierarchy in the name of conceptual purity. Right now the API is pretty flat and we only have inheritance where it is needed functionally, such as the subclasses of Signature like Sha256WithRsaSignature. Remember that this change would not only be for Java but all the languages including JavaScript where class hierarchies are not as clean.
Updated by Anonymous over 8 years ago
Andrew, any input on methods get/setControlParameters which are specific to a single ControlParameters in the ControlResponse body?
Updated by Andrew Brown over 8 years ago
Perhaps I was doing some creative interpretation, but I read http://redmine.named-data.net/projects/nfd/wiki/ControlCommand to mean that there could be multiple control parameters: " is the ControlParameters passed into this command for all successful responses." If there can only be one it would simplify the while-loop around line 90.
I think get/setControlParameters() may be confusing (e.g. does a ControlResponse have a status, a body, AND some ControlParameters); I would recommend naming it like encode/decodeBodyToControParameters() to make this more obvious. Not everyone is familiar with the NFD management protocol.
Another comment I have is that I had to do some buffer copying during encoding which I wish I could have avoided. I can't remember the exact details of why I had to do this; Jeff, did I miss something there? I don't think you have to do this inside TlvFormat?
Updated by Anonymous over 8 years ago
You say "naming it like encode/decodeBodyToControParameters". But encode/decode happens with wireEncode/Decode. I suppose you mean instead of get/setControlParameters, we should use get/setBodyAsControlParameters.
I suppose it doesn't hurt to have a list of ControlParameters. If the application expects only one, it can check. So following this and your other suggestion, I'll use get/setBodyAsControlParametersList. Sound good?
Updated by Alex Afanasyev over 8 years ago
Andrew, this is a language bug in the spec.
What is meant is that response returns ControlParameters that was supplied in the command. Speaking of NFD, there is exactly one ControlParameters structure in the command (and it doesn't make sense for me in general to have more than one) and there is no more than one ControlParameters structure in the response.
There is no value of having the list, especially if the structure hard-codes NFD-specific things.
Updated by Andrew Brown over 8 years ago
Jeff, your naming makes more sense (e.g. get/setBodyAsControlParameters)
Alex, sounds good.
Updated by Anonymous over 8 years ago
Hi Andrew. So we should limit to one ControlParameters (not a list)?
Updated by Andrew Brown over 8 years ago
Yes, Alex is assuring us there is only one ControlParameters and my only need for this is to interact with the NFD; the list isn't needed. I am assuming Alex will update the management spec to be a little more clear?
Updated by Anonymous over 8 years ago
- Status changed from New to In Progress
OK, I added ControlResponse in the branch issue/3455-ControlResponse.
https://github.com/named-data/jndn/blob/issue/3455-ControlResponse/src/net/named_data/jndn/ControlResponse.java
I pulled in your unit test and took the liberty of adding a ControlParameters to the test encoding. (I also updated the copyright, the same way we did for your TestRegistrationCallbacks.)
https://github.com/named-data/jndn/blob/issue/3455-ControlResponse/tests/src/net/named_data/jndn/tests/unit_tests/TestControlResponse.java
Updated by Alex Afanasyev over 8 years ago
You cannot simply change the copyright. TestControlResponse code is taken basically as is and the copyright belongs to Intel. ControlReposnse can go both ways, but many things there (e.g., documentation) copied from the original code.
Also, given there is no git history for these files, please add proper authorship to the files.
Updated by Jeff Burke about 8 years ago
JeffT, let's move the copyright discussion to email. The way this should work is the copyright should be kept intact for included code, as long as licenses are compatible.
Updated by Anonymous about 8 years ago
- Project changed from jndn to NDN-CCL
- % Done changed from 0 to 50
ControlResponse is merged to the master branch in jNDN. I changed the project to NDN-CLL since we should implement it in the other CCL libraries.
Updated by Anonymous about 8 years ago
- Subject changed from Import ControlResponse from jndn-management to Implement ControlResponse
Updated by Anonymous about 8 years ago
- Status changed from In Progress to Closed
- % Done changed from 50 to 100
ControlResponse is merged to the master branch in NDN-CPP, PyNDN and NDN-JS.