TLDR: I've abandoned the revert. After looking at Gibi's investigation further I agree that rabbit was actually using the jsonutils version of dumps, so making the fake driver use it is consistent. Apologies for the confusion. -Ben On 10/1/19 3:35 PM, Ken Giusti wrote:
Sorry I'm late to the party....
At the risk of stating the obvious I wouldn't put much faith in the fact that the Kafka and Amqp1 drivers use jsonutils. The use of jsonutils in these drivers is simply a cut-n-paste from the way old qpidd driver. Why jsonutils was used there... I dunno.
IMHO the RabbitMQ driver is the authoritative source for correct driver implementation - the Fake driver (and the others) should use the same serialization as the rabbitmq driver if possible.
-K
On Tue, Oct 1, 2019 at 4:30 AM Balázs Gibizer <balazs.gibizer@est.tech> wrote:
On Mon, Sep 30, 2019 at 5:35 PM, Balázs Gibizer <balazs.gibizer@est.tech> wrote: > > > On Mon, Sep 30, 2019 at 4:45 PM, Ben Nemec <openstack@nemebean.com <mailto:openstack@nemebean.com>> > wrote: >> Hi, >> >> I've just proposed https://review.opendev.org/#/c/685724/ which >> reverts a change that recently went in to make the fake driver in >> oslo.messaging use jsonutils for message serialization instead of >> json.dumps. >> >> As explained in the commit message on the revert, this is >> problematic >> because the rabbit driver uses kombu's default serialization method, >> which is json.dumps. By changing the fake driver to use jsonutils >> we've made it more lenient than the most used real driver which >> opens >> us up to merging broken changes in consumers of oslo.messaging. >> >> We did have some discussion of whether we should try to override the >> kombu default and tell it to use jsonutils too, as a number of other >> drivers do. The concern with this was that the jsonutils handler for >> things like datetime objects is not tz-aware, which means if you >> send >> a datetime object over RPC and don't explicitly handle it you could >> lose important information. >> >> I'm open to being persuaded otherwise, but at the moment I'm leaning >> toward less magic happening at the RPC layer and requiring projects >> to explicitly handle types that aren't serializable by the standard >> library json module. If you have a different preference, please >> share >> it here. > > Hi, > > I might me totally wrong here and please help me understand how the > RabbitDriver works. What I did when I created the original patch that > I > looked at each drivers how they handle sending messages. The > oslo_messaging._drivers.base.BaseDriver defines the interface with a > send() message. The oslo_messaging._drivers.amqpdriver.AMQPDriverBase > implements the BaseDriver interface's send() method to call _send(). > Then _send() calls rpc_commom.serialize_msg which then calls > jsonutils.dumps. > > The oslo_messaging._drivers.impl_rabbit.RabbitDriver driver inherits > from AMQPDriverBase and does not override send() or _send() so I think > the AMQPDriverBase ._send() is called that therefore jsonutils is used > during sending a message with RabbitDriver.
I did some tracing in devstack to prove my point. See the result in https://review.opendev.org/#/c/685724/1//COMMIT_MSG@11
Cheers, gibi
> > Cheers, > gibi > > > [1] > https://github.com/openstack/oslo.messaging/blob/7734ac1376a1a9285c8245a91cf... > >> >> Thanks. >> >> -Ben >> > >
-- Ken Giusti (kgiusti@gmail.com <mailto:kgiusti@gmail.com>)