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

JMS: Remove speparate Envelope wrapper by doubling message classes #1331

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

ennru
Copy link
Member

@ennru ennru commented Nov 15, 2018

Removes ProducerMessage.Envelope which wrapped all messages pass to the JmsProducerStage. Instead, all message classes themselves know about pass-throughs and have "pass-through-free" sub-classes to live without a type-parameter.

Surprisingly small change for the user.

@ennru
Copy link
Member Author

ennru commented Nov 16, 2018

But I wonder if all this is worth it. The overhead of JMS is so much bigger than anything else anyway.

@ennru
Copy link
Member Author

ennru commented Dec 20, 2018

@andreas-schroeder @akara Do you think this is a good idea?

@andreas-schroeder
Copy link
Contributor

@ennru I feel that the most distinguishing factor here are the downsides of the respective solutions. However, both variants have their downsides, and I don't see a clear winner.

  • The separate classes without pass-through and create a message model twice as large (so a bigger API surface), which I guess for a newcomer will feel like "does it really have to be like this...?" at a first glance.
  • The envelope wrapper I proposed creates overhead and feels oddly complicated. That being said, I went for this option because Akka Streams Kafka does it like this. That variant is therefore in-line with what was already there and might already be familiar.

So I don't have an opinion here, really :)

@ennru
Copy link
Member Author

ennru commented Jan 7, 2019

I find the user API with the combined message and envelope surprisingly small. The duplication in classes is hidden. I'm more afraid users may need more esoteric notations for the type parameters, especially in Java.
When you introduced the Envelope, it did not change existing API. This PR deliberatly does that.

@akara
Copy link
Contributor

akara commented Jan 7, 2019

I would not make this change just to save on the cost of the envelope object. Newer JVMs make both the creation and garbage collection cost of such objects more or less meaningless, especially compared to the cost and the potential message rate of JMS.

With performance concerns put away, I'd look at this change more of API aesthetics.

@akara
Copy link
Contributor

akara commented Jan 8, 2019

As it stands right now, we have the passthrough version of the message as a superclass and the non-passthrough version as a subclass. I'm wondering whether that is the right arrangement. We cannot take away a field in the subclass and therefore we mark it as NotUsed. Should it not be the opposite way?

Also, from a Scala perspective, I'd rather want to see the PassThrough as a separate trait. So JmsTextMessagePassThrough extends JmsTextMessage with PassThrough. I have not checked how this works out in Java, though. What do you think?

@ennru
Copy link
Member Author

ennru commented Jan 11, 2019

I agree that the inheritance Regular extends WithPathThrough[NotUsed] is a bit awkward. The tricky bit is to keep down the number of types the stage knows about.

I'll try and see if a trait solution would work.

@ennru
Copy link
Member Author

ennru commented Jan 15, 2019

I looked into this again and I don't think there is a good solution where we'd have a more natural inheritance for pass-throughs classes.
The user experience for this is my main interest and I believe it is very nice now. A user doesn't see the odd hierarchy and - for what I have tried - the generic types don't get in the way.

@2m
Copy link
Member

2m commented Jan 16, 2019

Needs a rebase.

Copy link
Member

@2m 2m left a comment

Choose a reason for hiding this comment

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

LGTM. I think streamlining the API especially the Java side is worth of this change.

@2m 2m merged commit f6c507f into akka:master Jan 16, 2019
@2m 2m added this to the 1.0-M2 milestone Jan 16, 2019
@ennru ennru deleted the ennru_jms-no-envelope branch January 16, 2019 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants