search

zeromq / malamute

The ZeroMQ Enterprise Messaging Broker
Mozilla Public License 2.0
324 stars 77 forks source link

Remove consumer from a stream #286

Closed wangkf1 closed 7 years ago

wangkf1 commented 7 years ago

I was looking into implementing this until issue #282 was created. This solution allows the removal of a consumer from a specified stream.

This is my first contribution to any zeromq project, so let me know if there is something I should do differently or change in order for the pull request to go through.

vyskocilm commented 7 years ago

Hi, this is neat. I have only one concern and this is backward compatibility. If new client will sent me command to old server, it will return invalid command, so client will silently break. Cc @bluca any ideas about malamute protocol evolution? We already have had a breakage post 1.0 release.

wangkf1 commented 7 years ago

Hm, I hadn't considered that possibility. I've only been using malamute for a few weeks, what happened the last time? And how was it resolved?

vyskocilm commented 7 years ago

It wasn't resolved, master can't talk to 1.0 server :-( We are learning as well.

I would say that the we should add a possibility for a client to declare the required version. Something like following code snippet. Server will check if supports required version and fail to connect if not. This does not solve backward compatibility, because server would need to return different states based on client version. Not sure if the whole protocol won't become messy because of that.

mlm_client_t *client = mlm_client_new ();
mlm_client_server_version (client, 42);
int r = mlm_client_connect (client, ....);
if (r != 0)
    zsys_error ("Can;t connect to broker");

Anyway please resolve conflicts and we can happily merge this PR

wangkf1 commented 7 years ago

I resolved the conflicts. It compiles and builds on my local computer and the checks run fine, but doesn't seem like it does on the CI build. One of them looks like is because the zproject generated file for mlm_proto.api doesn't match the file committed. That's because I noticed that, at least on my machine, zproject doesn't re-generate mlm_proto.api, so I manually changed it to compile and work. Has that happened before and is this a problem?

bluca commented 7 years ago

Please rebase instead of merging

wangkf1 commented 7 years ago

Sorry, I've never actually used rebase before. Should I revert the merge then?

vyskocilm commented 7 years ago

Hi, don't worry about history. We can squash commits during merge. Just drop the conflicts.

Dne 4. 10. 2017 6:33 odpoledne napsal uživatel "Kevin Wang" < notifications@github.com>:

Sorry, I've never actually used rebase before. Should I revert the merge then?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/zeromq/malamute/pull/286#issuecomment-334214186, or mute the thread https://github.com/notifications/unsubscribe-auth/AIa-1OBUkrNujbUbfItJMNqRSM_37Hwmks5so7NDgaJpZM4PsemV .

wangkf1 commented 7 years ago

So I should hard reset my history to before the merge, then do a rebase instead? I don't know what you mean by squashing commits

wangkf1 commented 7 years ago

Ok, I see why you'd prefer a rebase. I've always done merges since I haven't forked a repo before. Done!

As I mentioned above, are the TravisCI errors a big deal?

vyskocilm commented 7 years ago

Hi, I will try to review it today evening.

Dne 4. 10. 2017 6:52 odpoledne napsal uživatel "Kevin Wang" < notifications@github.com>:

Ok, I see why you'd prefer a rebase. I've always done merges since I haven't forked a repo before. Done!

As I mentioned above, are the TravisCI errors a big deal?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/zeromq/malamute/pull/286#issuecomment-334219869, or mute the thread https://github.com/notifications/unsubscribe-auth/AIa-1MHZI73QGPYj01ngSKPNepbpW6dCks5so7fogaJpZM4PsemV .

vyskocilm commented 7 years ago

Fail is due https://github.com/zeromq/libzmq/issues/2762

Assertion failed: zap_required() (src/plain_server.cpp:50)

We can merge this one.

wangkf1 commented 7 years ago

Awesome, thanks!

vyskocilm commented 7 years ago

This is because stream cancel message has been add into the middle of the file. Fixed in #288

Dne 5. 10. 2017 7:52 odpoledne napsal uživatel "Luca Boccassi" < notifications@github.com>:

@bluca commented on this pull request.

In include/mlm_proto.h https://github.com/zeromq/malamute/pull/286#discussion_r143010041:

@@ -159,17 +163,18 @@ indicates that the message could not be delivered.

define MLM_PROTO_CONNECTION_CLOSE 4

define MLM_PROTO_STREAM_WRITE 5

define MLM_PROTO_STREAM_READ 6

-#define MLM_PROTO_STREAM_SEND 7 -#define MLM_PROTO_STREAM_DELIVER 8 -#define MLM_PROTO_MAILBOX_SEND 9 -#define MLM_PROTO_MAILBOX_DELIVER 10 -#define MLM_PROTO_SERVICE_SEND 11 -#define MLM_PROTO_SERVICE_OFFER 12 -#define MLM_PROTO_SERVICE_DELIVER 13 -#define MLM_PROTO_OK 14 -#define MLM_PROTO_ERROR 15 -#define MLM_PROTO_CREDIT 16 -#define MLM_PROTO_CONFIRM 17 +#define MLM_PROTO_STREAM_CANCEL 7

@wangkf1 https://github.com/wangkf1 any specific reason why this couldn't be number 18? This change causes an ABI breakage

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/zeromq/malamute/pull/286#pullrequestreview-67459002, or mute the thread https://github.com/notifications/unsubscribe-auth/AIa-1JREWQV5ceDNYSf1S5TzXAW1spcUks5spRdLgaJpZM4PsemV .