> 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. > > Rafael Schloming wrote: > 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?
Ok, so as long as standard proton.Message has a send with the right defaults, I'd be fine with that. I'll update the patch. - Gordon ----------------------------------------------------------- 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 > >
