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.

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

- Adds ability to associate arbitrary data with a Cro::Message #38

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

Xliff
Copy link
Contributor

@Xliff Xliff commented Jul 3, 2024

No description provided.

@@ -2,6 +2,26 @@
# application is processing. It might be a message from a message queue, a
# ZeroMQ message, a HTTP request, a HTTP response, etc.
role Cro::Message {
also does Associative;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It's the easiest way to implement what I need.

# cw: For OOB attributes associated with the messages. Consider
# that there is no way to know who the message is from,
# or where it is supposed to go.
has %!attributes;
Copy link

@lizmat lizmat Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not has %!attributes handles <keys pairs values AT-KEY EXISTS-KEY> and possibly extend on that when needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should work.

@vendethiel
Copy link
Member

Can't you just mix in a role at point?

@patrickbkr
Copy link
Contributor

I'd like a bit more info on the use case this tries to solve.

@Xliff
Copy link
Contributor Author

Xliff commented Jul 10, 2024

A role? No.

The point is that... as far as a Cro::Message goes, there is ONLY the payload. There isn't any way to determine routing information. I can't tell where the Message came from or where it is going to!

Consider: Any use case that isn't a microservices server that use TCP as a transport mechanism.

To make things worse, Cro::Message doesn't even allow you to look at the TCP data to get said routing information.

And what if the routing needs go beyond TCP?

So in this situation, I decided to make Cro::Message capable of handling any set of OOB information (OOB = anything outside of the message payload).

@patrickbkr
Copy link
Contributor

In the case of Cro::HTTP Cro::Message is inherited into Cro::HTTP::Message adding fields as necessary. Wouldn't the same also work for your use case?

Should we decide this is a good idea, I would like to not have Cro::Message act as an Associative. It's a pretty arbitrary decision that user defined data should be accessible via the Associative interface while other mappy bits (like headers or cookies in an HTTP::Message) are accessed using attributes. This creates confusion. Can we instead just have has %.attributes;?

@patrickbkr
Copy link
Contributor

In Cro-FCGI I subclassed Cro::HTTP::Request adding a few FCGI specific fields and have the Cro::FCGI::RequestParser emit objects of that type. That worked for my use case.

@Xliff
Copy link
Contributor Author

Xliff commented Jul 10, 2024

I'm OK with dropping the Associative interface, but I do need out of band data in a Cro::Message subclass.

UPDATE - After looking at Cro::HTTP::Message, I am still of the opinion that Cro::Message is the best place for this. Remember, Cro::HTTP::Message is for HTTP traffic. I need a generic solution that does not tie itself to any specific transport.

Why is there a problem with putting %.attributes on Cro::Message?

Also, I am not OK with making this a subclass. This is something I feel is missing from Cro::Message, and I have yet to hear a good reason as to why it shouldn't be there.

Thanks for listening.

@patrickbkr
Copy link
Contributor

I don't have a big issue with that attribute. It's just that adding public interfaces is a lot easier than removing them. And as I don't have a good intuition about the Cro APIs yet, I'm a bit cautious to add public interfaces especially in the lower layers. That's all.

So given we get rid of Associative, I'm ok with merging this.

@Xliff
Copy link
Contributor Author

Xliff commented Jul 11, 2024

Thank you @patrickbkr - Associative role removed.

@patrickbkr patrickbkr merged commit c70029f into croservices:master Jul 11, 2024
0 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants