> On Jan. 27, 2015, 7:37 p.m., Rafael Schloming wrote: > > proton-c/bindings/python/proton/__init__.py, line 2701 > > <https://reviews.apache.org/r/30318/diff/1/?file=836290#file836290line2701> > > > > This seems to change the semantics of send in a subtle but odd way. If > > you pass in something that it knows how to encode into bytes, it will > > assume that constitutes an entire message and call advance and > > automatically generate a tag for you. Yet if you pass in raw bytes, it > > doesn't do this. I think these two things should really be controlled > > orthogonally, e.g. I might want to be able to configure the sender to > > assume every call to send is a single message even if I'm just passing in > > raw bytes, and I might also want to be able to stream large messages out by > > passing in higher level message chunks that the sender automatically > > encodes into bytes for me. > > Gordon Sim wrote: > WHat I want is a simple, single call to send a message. I'd like that to > be called send(), since that seems a clear and intuitive name for it. However > that clashes with the existing definition. I would argue that send() isn't > such a great name for the current semantics in that it isn't (in my view) > clear that you first need to create a delivery and then need to call > advance() when done. To me the current semantic is more like an append_data(). > > However, I didn't want to impose a breaking change. Hence the somewhat > odd combination of semantics for the same name. > > There are (at least) three ways to deal with this: > > (1) use send_message() for the new simple, single call semantic and leave > send() as is. I could live with that but it seems a shame to me to use the > obvious name for the less obvious (and I believe less commonly required) > semantic > > (2) use send() exclusively for the new simple, single call semantic, and > have send_bytes() - or better still perhaps, append_data() - as the old > semantic. This is obviously a breaking API change though, not sure how > acceptable that would be? > > (3) add in some flag(s)/option(s) to control the single-shot v. > part-of-sequence-of-calls semantics. However the defaults need to be decided. > So agin, we can default it to the current bheaviour, but that in my view is > less intuitive and less commonly what users want. Or we can default it to the > new behaviour, but that then is another breaking change. > > To me, if a breaking change is acceptable, I'd probably go for something > like (2), because I think it will be clearer. Perhaps I'm just not > imaginative enough yet on what the flags/options might be like in 3. > > If a breaking change isn't acceptable, I'd probably rather just stick > with a send_message() rather than have a send() where a non-default option > was required to trigger the single-shot behaviour. > > Rafael Schloming wrote: > I agree with you that send(Message) should do the obvious thing, so I'm > not keen on (1). I don't agree that send is a bad name for the streaming > behaviour though. I think in the scenario where you are streaming a large > message out over the wire, you are still very much sending, it's just that > you are sending "Message Chunks" rather than entire messages, but you are > still sending a unit of work that you expect to make it all the way over the > wire to the receiver. > > I also think that automatically rendering objects into amqp bytes > shouldn't be only associated with the non streaming case, in other words I > want to be able to do something like this: > > send(Header); send(Chunk); ...; send(Footer) > > Where each of those things are actual objects that know how to render > themselves into the appropriate AMQP section. > > One possibility here might be to delegate back to the object itself again > for a bit more flexibility, e.g. have send check if the object being sent has > some sort of "send_yourself" method and then delegate back to it. That would > allow the object to then set up whatever delivery tags it wanted, encode > itself however it wanted, and then choose whether or not to advance the link. > Presumably in the above example the Header would set up the delivery tag, the > Chunk would just send more data, and the Footer would advance the link. > > That of course does leave the question of defaults when you pass in raw > bytes. To keep backwards compatibility we would need to say that passing in > raw bytes is equivalent to passing in Chunk(bytes), assuming Chunk is defined > to be roughly what I described above. I'm not opposed to breaking > compatibility here though, I think that's partly the point of this release. > > Gordon Sim wrote: > I fully agree that automatically encoding an object into bytes shouldn't > be restricted to a Message. > > In theory I also agree that the object being sent could identify whether > it is 'complete' or not. My only concern there would be that it might pollute > the Message class with internal detail. One could argue that at present, the > code recognises that a Message object is self contained. You could replace > that by some other scheme to determine whether the object includes the > 'start' and/or the 'end', or simply whether it 'is_complete', but as I say, > it feels a little ugly to add that to Message. Do you have a suggestion that > you think would be nice there? To me having separate flags as extra arguments > to the method feels nicer. > > I'm also less convinced that the single object should also provide the > tag though. I think it should be possible to separate the strategy used to > generate delivery tags from the messages sent. (Maybe I'm misunderstanding > the suggestion here though). > > Rafael Schloming wrote: > I'm not quite sure I follow what you mean by 'internal detail'. I'm > picturing something like this: > > class MyMessage: > ... > def send(self, sender): > tag = sender.generate_tag_for_me() > # or alternatively > tag = self.id > # or maybe > tag = self.id + '::' + sender.sequece() > > dlv = sender.delivery(tag) > > sender.send(self.encode()) > # or possibly depending on backwards compatibility options and what > we want send(bytes) to do by default > sender.send_bytes(self.encode()) > > sender.advance() > # or maybe even depeding on the QoS of the message > dlv.settle() # I think this could be a nice option to have since > forgetting to settle deliveries is something of a gotcha that you don't > notice until they start piling up. I know I've been caught by it a bunch of > times and I know the API pretty well. > > return dlv > > Sender.send would then become: > > class Sender: > ... > def send(self, obj): > if hasattr(obj, "send"): > return obj.send(self) > else: > ... > > Certainly it adds a dependency from MyMessage onto the Sender interface, > but it's entirely in terms of the public interface. You could as you say flip > it the other way around: > > class MyMessage: > ... > def render(self): > return self.encode(), self.tag(), self.is_start(), self.is_end() > > class Sender: > ... > def send(self, obj): > if hasattr(obj, "render"): > blah = obj.render() > # use blah > else: > ... > > Or something like that, but this seems less flexible. > > I'm also fine with using flags as extra arguments, i.e. saying the basic > send is something like: > > def send(self, bytes, tag=None, start=True, stop=True): > ... > > And if you pass in an object rather then a string we query that object to > default all the subsequent fags. I don't think it's very nice to just default > bytes and tag and leave start/stop as undefaultable. It seems inconsistent > and it leaves the streaming scenario as a second class citizen for no reason. > > My point on the tag generation is not that the message alone should > provide the tag, but if the message has a natural message id, e.g. say a UUID > or something like that, then it is nice to be able to include that in the > tag. The way you have it set up currently the tag generation scheme has no > access to the message, and so it wouldn't be able to do that. In other words > whatever code is generating the tag should really have access to both the > message and the sender (or some strategy associated with the sender). > > Gordon Sim wrote: > The way I have it, tags are generated by a sender scoped generator, which > could be set to be anything. To me that is 'some strategy associated with the > sender'. > > I'm not clear who would provide the MyMessage here? Is that something an > application would have to do? > > Perhaps what we could do is query for a send method, which if it exists > gets called and nothing further is done. If not we query for > tag/is_start/is_end methods and use the returned values if they exist. If not > we fallback to the method arguments. Then for the encode again, we check for > an encode method. > > That way you can send() a standard proton.Message with a single call, > which I think is most intuitive, but there is also scope for other objects > that customise things as necessary. > > Does that sound reasonable? If so I can update the patch.
I just put in MyMessage to illustrate that an application could do it if it wanted to. I'm assuming that Message would override it in such a way as to provide the simple send semantics. If you query for the send method and delegate to it as you describe, I'm not sure why the other steps would be necessary. Am I missing something? - Rafael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30318/#review69863 ----------------------------------------------------------- On Jan. 27, 2015, 2 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30318/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2015, 2 p.m.) > > > Review request for qpid and Rafael Schloming. > > > Repository: qpid-proton-git > > > Description > ------- > > Sending a message requires several steps: create a delivery with a unique > tag, encode the message, cal send(), call advance. While this gives a lot of > flexibility and power (e.g. streaming messages in chunks without ever holding > the entire thing in memory), for many, many use cases, a simpler solution > would be adequate and would avoid users having to learn what is in my view > one of the more esoteric aspects of the API. > > The attached patch adds send support for a Message object, such that a > message can be sent with a single call. It also allows the send() method to > be 'dual purpose'. It remains backwards compatible as a way of appending > bytes to the current delivery, but can also be used as a one-step call to > send a single message. > > At present the engine examples in python make use of a similar > simplification, but one that is added dynamiccaly and is therefore less clean > and obvious. It currently also doesn't work as well for sending on links > established by the remote peer. This addition would in my view allow for > simpler use where needed, without breaking any existing usage. > > > Diffs > ----- > > proton-c/bindings/python/proton/__init__.py 17cef30 > > Diff: https://reviews.apache.org/r/30318/diff/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
