Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improvements #214

Closed
wants to merge 3 commits into from
Closed

Improvements #214

wants to merge 3 commits into from

Conversation

diorcety
Copy link

@diorcety diorcety commented Sep 16, 2016 •
edited
Loading

Unify the way to send/receive a message.
Currently, stream api is different than service/mailbox. The client must register itself as producer to the server (related to only one stream). Instead of that you can send the stream with the message itself. This will allow a client to send a message on multiple streams as service/mailbox
This PR is more a POC or a Issue entry with some example code, there is certainly missing binding changes to do. related to #208

@diorcety
Copy link
Author

diorcety commented Apr 4, 2017

Hi,
I have no comment on this PR (yes i know that the CI fails). Are you interrested in it. For me it seems to be a good refactoring in order to have the same API for each kind of service. If you are interrested I will try to fix the CI issues

@bluca
Copy link
Member

bluca commented Apr 4, 2017

Hi,

Thanks for the PR, but please note we follow the C4 process: https://rfc.zeromq.org/spec:42/C4/

It would be better to avoid breaking the existing APIs, as that is a major pain for users. And when that is not possible, we mark the old one as deprecated (but keep it there for a long transitional period) and add a new one.

@diorcety
Copy link
Author

diorcety commented Apr 5, 2017 •
edited
Loading

Ok no problem, I understand. The goal is to consolidate the API, which can be tricky with the need to deprecated the old one. I am opened to recomandations

@vyskocilm
Copy link
Contributor

Hi - I agree with @bluca - the best is to add an another messaging pattern ("MULTISTREAM"??). I am closing the PR.

@vyskocilm vyskocilm closed this Jul 19, 2017
@diorcety
Copy link
Author

Okay. As the API is un draft mode, i was thinking that was a good idea to uniformize the API.

@bluca
Copy link
Member

bluca commented Jul 19, 2017

Oh I thought it was stable? @vyskocilm didn't we mark all current malamute APIs as stable a while ago?

@diorcety
Copy link
Author

Some places as the headers descibe the API as draft.

@lerwys
Copy link
Contributor

lerwys commented Jul 19, 2017 •
edited
Loading

@bluca @diorcety Sorry for jumping in this thread, but from what I recall Malamute API was never marked as stable, not even after v1.0 release. I think we are long due to mark this API as stable by making another release.

As there are users already relying on this API (at least I do) I think it would be good to wait until we make another release with the current API marked as stable and after that we could think on changing the API to something else or adding another messaging pattern as @vyskocilm suggested.

@vyskocilm
Copy link
Contributor

vyskocilm commented Jul 19, 2017 via email

@bluca
Copy link
Member

bluca commented Jul 19, 2017

Yes please do, I honestly thought it already was as part of 1.0, sorry for the confusion @diorcety

@lerwys
Copy link
Contributor

lerwys commented Jul 19, 2017

I think we need at least to bump minor version, right?

@bluca
Copy link
Member

bluca commented Jul 19, 2017

From the user's point of view there's no change, so it's not urgent to do a tagged release

@diorcety
Copy link
Author

I have no problem at all on this position. This PR is only the result of my experience with malamute, with one client I can call or provide multiple services, I can listen multiples streams but I can't provide multiples streams.

@diorcety
Copy link
Author

I am agree with jour position because i can see the backward compatibily as an issue, It seems there is no tech pitfall.

@lerwys
Copy link
Contributor

lerwys commented Jul 19, 2017 •
edited
Loading

@bluca But that''s just because malamute has only draft APIs maybe. If it had an stable API, draft API and the user built without --enable-drafts=yes (which I think is the default), promoting the draft API to stable would change the user's perspective, as new methods would be enabled by default.

Maybe I'm just being too pedantic about tagged releases? =]

@bluca
Copy link
Member

bluca commented Jul 19, 2017

Yeah you are indeed right, we should do 1.1

@bluca
Copy link
Member

bluca commented Jul 19, 2017

Actually given there were only DRAFT classes, DRAFT APIs were enabled by default so there should not be any chances:

#   Project has no stable classes so enable draft API by default
enable_drafts=yes

@lerwys
Copy link
Contributor

lerwys commented Jul 19, 2017

Hum... yes. It seems that in this case --enable-drafts was not needed as you said.

Regardless, I think it would be a good thing to bump the minor version as to indicate an API left DRAFT state as went into STABLE.

The case with the --enable-drafts on by default seems to me more as a convenience to the user, as building without it would render the generated binary useless.

@bluca
Copy link
Member

bluca commented Jul 19, 2017

Sure. If you guys give it a good testing, tomorrow I can tag a release.

@lerwys
Copy link
Contributor

lerwys commented Jul 19, 2017

Okey. No problem. I will test against the current master to be sure.

@lerwys
Copy link
Contributor

lerwys commented Jul 19, 2017 •
edited
Loading

Actually, there's a minor issue in file src/malamute.c:

#include "mlm_classes.h"

#define PRODUCT         "Malamute service/0.2.0"
#define COPYRIGHT       "Copyright (c) 2014-16 the Contributors"

Instead it should be something like:

#include "mlm_classes.h"

#define STRINGIFY(s) PRIMITIVE_STRINGIFY(s)
#define PRIMITIVE_STRINGIFY(s) #s

#define MLM_VERSION_MAJOR_STR STRINGIFY(MLM_VERSION_MAJOR)
#define MLM_VERSION_MINOR_STR STRINGIFY(MLM_VERSION_MINOR)
#define MLM_VERSION_PATCH_STR STRINGIFY(MLM_VERSION_PATCH)

#define PRODUCT         "Malamute service/" MLM_VERSION_MAJOR_STR"."MLM_VERSION_MINOR_STR"."MLM_VERSION_PATCH_STR
#define COPYRIGHT       "Copyright (c) 2014-16 the Contributors"

@bluca
Copy link
Member

bluca commented Jul 19, 2017

Could you please send a PR?

@lerwys
Copy link
Contributor

lerwys commented Jul 19, 2017

Sure. I will send it in a minute.

@lerwys
Copy link
Contributor

lerwys commented Jul 21, 2017

@bluca At least from my side malamute commit 540b497 is working fine.

@bluca
Copy link
Member

bluca commented Jul 21, 2017

One possible issue is that there have been a few changes in the protocol between client and server:

v1.0...master

So I wonder if interoperability has been broken since 1.0? Does a 1.0 client still work with a 1.1 server and viceversa?

@lerwys
Copy link
Contributor

lerwys commented Jul 21, 2017

Yes, you're right, but we only need to keep compatibility from v1.0 -> v1.1, I think, right?
Backwards compatibility between minor versions (v1.1 -> v1.0) would not need to be guaranteed, if that's the case.

Anyway, Do you think running the v1.0 client self-tests against a v1.1 server should be enough?

@bluca
Copy link
Member

bluca commented Jul 22, 2017

The selftest will use the same library for server&client, so it will not expose protocol problems.
The only way is if anyone has an application, and can test one side with 1.0 and the other side with master.

@vyskocilm can you guys help?

@vyskocilm
Copy link
Contributor

vyskocilm commented Jul 22, 2017 via email

@bluca
Copy link
Member

bluca commented Jul 22, 2017

thanks

@karolhrdina
Copy link
Contributor

Hi @bluca,
I tried to run 1.0 client selftest agains 1.1 malamute server, using the following steps

  • git worktree add ./old_mlm v1.0
  • built both projects
  • modified old/src/mlm_client.c selftest to use ipc endpoint
  • modified root src/malamute.cfg to use the same ipc endpoint

However it fails:

 * mlm_client: 
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=$CONNECTED
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=SET PRODUCER
D: 17-07-22 19:00:27 544759:mlm_client                      : start:
D: 17-07-22 19:00:27 544759:mlm_client                      :     set_producer
D: 17-07-22 19:00:27 544759:mlm_client                      :         $ signal failure
D: 17-07-22 19:00:27 544759:mlm_client                      :         > start
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=$CONNECTED
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=SET CONSUMER
D: 17-07-22 19:00:27 544759:mlm_client                      : start:
D: 17-07-22 19:00:27 544759:mlm_client                      :     set_consumer
D: 17-07-22 19:00:27 544759:mlm_client                      :         $ signal failure
D: 17-07-22 19:00:27 544759:mlm_client                      :         > start
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=$CONNECTED
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=SET WORKER
D: 17-07-22 19:00:27 544759:mlm_client                      : start:
D: 17-07-22 19:00:27 544759:mlm_client                      :     set_worker
D: 17-07-22 19:00:27 544759:mlm_client                      :         $ signal failure
D: 17-07-22 19:00:27 544759:mlm_client                      :         > start
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=$CONNECTED
D: 17-07-22 19:00:27 544759:mlm_client                      :     API message=$FLUSH
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=DESTRUCTOR
D: 17-07-22 19:00:27 544759:mlm_client                      : start:
D: 17-07-22 19:00:27 544759:mlm_client                      :     destructor
D: 17-07-22 19:00:27 544759:mlm_client                      :         $ signal success
D: 17-07-22 19:00:27 544759:mlm_client                      :         $ terminate
D: 17-07-22 19:00:27 544759:mlm_client                      :         > start
D: 17-07-22 19:00:27 544759:mlm_client                      :     API command=$TERM
D: 17-07-22 19:00:27 mlm_client_test:     API command=LOAD
N: 17-07-22 19:00:27 server is using PLAIN security
I: 17-07-22 19:00:27 zauth: API command=PLAIN
W: 17-07-22 19:00:27 could not bind to  (Invalid argument)
D: 17-07-22 19:00:27 865342:mlm_client                      :     API command=SET PLAIN AUTH
D: 17-07-22 19:00:27 865342:mlm_client                      : start:
D: 17-07-22 19:00:27 865342:mlm_client                      :     set_plain_auth
D: 17-07-22 19:00:27 865342:mlm_client                      :         $ use plain security mechanism
D: 17-07-22 19:00:27 865342:mlm_client                      :         $ signal success
D: 17-07-22 19:00:27 865342:mlm_client                      :         > start
D: 17-07-22 19:00:27 865342:mlm_client                      :     API command=CONNECT
D: 17-07-22 19:00:27 865342:mlm_client                      : start:
D: 17-07-22 19:00:27 865342:mlm_client                      :     connect
D: 17-07-22 19:00:27 865342:mlm_client                      :         $ connect to server endpoint
D: 17-07-22 19:00:27 865342:mlm_client                      :         $ set client address
D: 17-07-22 19:00:27 865342:mlm_client                      :         $ use connect timeout
D: 17-07-22 19:00:27 865342:mlm_client                      :         $ send CONNECTION_OPEN
D: 17-07-22 19:00:27 865342:mlm_client                      :         > connecting
D: 17-07-22 19:00:28 865342:mlm_client                      : connecting:
D: 17-07-22 19:00:28 865342:mlm_client                      :     expired
D: 17-07-22 19:00:28 865342:mlm_client                      :         $ signal server not present
D: 17-07-22 19:00:28 865342:mlm_client                      :         > start
mlm_selftest: src/mlm_client.c:436: mlm_client_test: Assertion `rc == 0' failed.
Makefile:1803: recipe for target 'check-verbose' failed
make: *** [check-verbose] Aborted (core dumped)

@bluca
Copy link
Member

bluca commented Jul 22, 2017

Ok, thanks for testing it.
This is the diff in the zproto definitions since 1.0:

diff --git a/src/mlm_client.xml b/src/mlm_client.xml
index 07406cc..fb1e0e0 100644
--- a/src/mlm_client.xml
+++ b/src/mlm_client.xml
@@ -16,6 +16,8 @@
             <action name = "signal success" />
         </event>
         <event name = "connect" next = "connecting">
+            <!-- This action should be called before "set client address" -->
+            <action name = "remember client address" />
             <action name = "connect to server endpoint" />
             <action name = "set client address" />
             <action name = "use connect timeout" />
@@ -47,8 +49,8 @@
 
     <state name = "connecting" inherit = "defaults">
         <event name = "OK" next = "connected">
-            <action name = "signal success" />
             <action name = "client is connected" />
+            <action name = "signal success" />
         </event>
         <event name = "expired" next = "start">
             <action name = "signal server not present" />
@@ -68,9 +70,12 @@
             <action name = "prepare service offer command" />
             <action name = "send" message = "SERVICE OFFER" />
         </event>
-        <event name = "destructor" next = "disconnecting">
+        <event name = "destructor" next = "terminating">
             <action name = "send" message = "CONNECTION CLOSE" />
         </event>
+        <event name = "bad pattern">
+            <action name = "signal bad pattern" />
+        </event>
         <event name = "STREAM DELIVER">
             <action name = "pass stream message to app" />
         </event>
@@ -94,10 +99,13 @@
             <action name = "signal failure" />
             <action name = "check status code" />
         </event>
+        <!-- if we receive PONG from server, we still need to wait for the response from the server -->
+        <event name = "CONNECTION PONG">
+        </event>
     </state>
 
     <!-- After client calls destructor -->
-    <state name = "disconnecting" inherit = "defaults">
+    <state name = "terminating" inherit = "defaults">
         <event name = "OK">
             <action name = "signal success" />
             <action name = "terminate" />
@@ -106,6 +114,13 @@
             <action name = "signal failure" />
             <action name = "terminate" />
         </event>
+        <!-- Even if we still receive PONGS from server, we are going to shutdown -->
+        <event name = "CONNECTION PONG">
+        </event>
+        <event name = "ERROR">
+            <action name = "signal failure" />
+            <action name = "terminate" />
+        </event>
     </state>
 
     <!-- After server sends ERROR for our PING -->
@@ -131,6 +146,10 @@
             <action name = "get next replay command" />
         </event>
         <event name = "replay ready" next = "connected">
+            <action name = "client is connected" />
+        </event>
+        <!-- if we receive PONG from server, we still need to wait until replay would finish -->
+        <event name = "CONNECTION PONG">
         </event>
     </state>
 
@@ -148,10 +167,6 @@
         <event name = "set worker">
             <action name = "signal failure" />
         </event>
-
-        <event name = "heartbeat">
-            <!-- Do not keep sending PONGs when disconnected -->
-        </event>
         <event name = "destructor">
             <action name = "signal success" />
             <action name = "terminate" />
@@ -179,6 +194,7 @@
 
     <state name = "have error">
         <event name = "command invalid" next = "reconnecting">
+            <action name = "set client address" />
             <action name = "use connect timeout" />
             <action name = "send" message = "CONNECTION OPEN" />
         </event>
@@ -187,7 +203,7 @@
             <action name = "terminate" />
         </event>
         <event name = "other">
-            <action name = "signal unhandled error" />
+            <action name = "announce unhandled error" />
             <action name = "terminate" />
         </event>
     </state>
diff --git a/src/mlm_server.xml b/src/mlm_server.xml
index 481843c..5d7f0e7 100644
--- a/src/mlm_server.xml
+++ b/src/mlm_server.xml
@@ -110,8 +110,10 @@
         <event name = "stream message">
             <action name = "get message to deliver" />
         </event>
+        <!-- All other protocol messages are invalid, tell this to the client -->
         <event name = "*">
-            <!-- Ignore any other commands -->
+            <action name = "signal command invalid" />
+            <action name = "send" message = "ERROR" />
         </event>
     </state>
 </class>

What do we do? The mlm protocol is not versioned at the moment, perhaps we should, like we do for ZMTP and then have the same restrictions on modifications?

Or do we instead declare that the protocol is an internal matter, and that compatibility is not guaranteed between different versions of libmlm?

I don't use malamute in production but IIRC you guys all do, so in the end it's up to you.

@karolhrdina
Copy link
Contributor

karolhrdina commented Jul 22, 2017 •
edited
Loading

I vote for Option #1:
IMHO compatibility should be guaranteed in between versions and i agree with your suggestion to version the protocol and apply certain rules to it's modification, just like with ZMTP.

@lerwys
Copy link
Contributor

lerwys commented Jul 22, 2017 via email

@bluca
Copy link
Member

bluca commented Jul 22, 2017

Yes ZMTP has its own RFC and versions: https://rfc.zeromq.org/spec:23/ZMTP/
libzmq is just a 3.0 compliant implementation.

As you guessed, MLM would have its own RFC and so on.

@lerwys
Copy link
Contributor

lerwys commented Jul 22, 2017 via email

@vyskocilm
Copy link
Contributor

vyskocilm commented Jul 22, 2017 via email

@lerwys
Copy link
Contributor

lerwys commented Jul 26, 2017

I'm not 100% familiar with Malamute protocol and, for this reason, I was thinking a more experienced contributor would be better suited to make this.

Anyway, I will try to give it a shot at this. The RFCs are collected in this repository right? https://github.com/zeromq/rfc

I will open another issue regarding the Malamute RFC so we can discuss there ok? The original subject of this topic has been greatly abused. =]

@lerwys lerwys mentioned this pull request Jul 26, 2017
Open
@BHare1985 BHare1985 mentioned this pull request Mar 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@diorcety @bluca @vyskocilm @lerwys @karolhrdina