Skip to content

Support hints for codec on client and server side #26220

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

Closed

Conversation

rudy2steiner
Copy link

@pivotal-issuemaster
Copy link

@rudy2steiner Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@rudy2steiner Thank you for signing the Contributor License Agreement!

@rudy2steiner
Copy link
Author

@rstoyanchev would you mind helping me review this PR!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 5, 2020
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

@rudy2steiner thanks for those changes but I'm afraid it's not going to work out in its present form. What I had in mind was a lot less extensive, simply passing MessageHeaders in PayloadMethodArgumentResolver as a hint under a well-known key so that a custom Decoder can take into account message headers, including extracted metadata.

I don't think RSocketRequester should be involved in any of these changes. Composite metadata already supports sending metadata of any mime type, and there is no reason for it to know what kind of metadata it is.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Dec 7, 2020
@rstoyanchev rstoyanchev self-assigned this Dec 7, 2020
@rudy2steiner
Copy link
Author

rudy2steiner commented Dec 8, 2020

@rudy2steiner thanks for those changes but I'm afraid it's not going to work out in its present form. What I had in mind was a lot less extensive, simply passing MessageHeaders in PayloadMethodArgumentResolver as a hint under a well-known key so that a custom Decoder can take into account message headers, including extracted metadata.

I don't think RSocketRequester should be involved in any of these changes. Composite metadata already supports sending metadata of any mime type, and there is no reason for it to know what kind of metadata it is.

Yes, I see what you have in mind. Once we pass headers as hints to decoder, we could add any metadata with self-defined mime on client side, and the metadata will be available for server decoder. But how to keep encoder of client and decoder of server have same hints context is a problem.

I have to admit the current changes isn't the best solution for the issue .

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 8, 2020
@rudy2steiner
Copy link
Author

May we could introduce a new method on RequestSpec:
data with hints for local encoders

RetrieveSpec data(Object data, Map<String,Object> hints );

and hints for remote decoder could implement by add a metadata use MetadataSpec#metadata

In this way, only a few changes and providing the ability to attach hints for encoder and decoder

@rstoyanchev
Copy link
Contributor

But how to keep encoder of client and decoder of server have same hints context is a problem.

I don't follow what you mean. Wouldn't the client just add some metadata which then the server can use for decoding?

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Dec 10, 2020
@rudy2steiner
Copy link
Author

rudy2steiner commented Dec 11, 2020

I don't follow what you mean. Wouldn't the client just add some metadata which then the server can use for decoding?

Do you think we should provide the ability for add hints for client encoding at the same time?

Client encoding always use a EMPTY_HINTS on DefaultRequestSpec#encodeData:

private <T> encodeData(T value, ResolvableType elementType, @Nullable Encoder<?> encoder) {
			if (encoder == null) {
				elementType = ResolvableType.forInstance(value);
				encoder = strategies.encoder(elementType, dataMimeType);
			}
			return ((Encoder<T>) encoder).encodeValue(
					value, bufferFactory(), elementType, dataMimeType, EMPTY_HINTS);
}

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 11, 2020
@rstoyanchev
Copy link
Contributor

I think it would be useful if you could provide more details of what you are trying to achieve more concrete. That would help to think about possible solutions.

@rudy2steiner
Copy link
Author

I think it would be useful if you could provide more details of what you are trying to achieve more concrete. That would help to think about possible solutions.

In my case, the hints will be used to affect behavior of encoder and decoder, but found that hints are always empty. I need to rewrite a lot of code to implement the requirement by myself, but I think Spring Messaging should built-in support user to add hints for encoder and decoder for per message , the hints are possibly different for per message.

We could discuss how to implement this feature if you agree with this would benefit for user.

@rstoyanchev
Copy link
Contributor

I'm afraid that doesn't tell me much more. I gather that you want to pass hints but what's not clear is why you want to pass hints into a client Encoder that would then be serialized and passed to the server side as well. By "provide more details" I meant provide a concrete scenario to illustrate the use case.

@rudy2steiner
Copy link
Author

rudy2steiner commented Dec 18, 2020

I have messages from different sources and be wrapped into instance of same class, but I want to serialize it using different algorithm for message from different source .

If I can pass a hint that indicate where the message from, then selecting corresponding algorithm to encode on client and decode it on server.

So I need a way to pass a hint for local encoder and remote decoder.

As i can seen, Hints as a metadata can be abstracted from headers and then passing it to decoder.
at the same time, I need pass the same hint to local encoder.

Spring messaging has passed the hints into encoder and decoder, but it's alway empty and can't be customized by users.

@rstoyanchev
Copy link
Contributor

Sorry for the delay and thanks for the extra details. This is quite helpful although it's still a bit abstract. It would help to have a little more detail about the different encoding algorithms. Do they result in a different format on the wire for example, which could potentially be handled by using different data MIME types. Or perhaps something in the Object structure or type could be used to differentiate?

@rstoyanchev rstoyanchev added type: enhancement A general enhancement in: messaging Issues in messaging modules (jms, messaging) labels Jan 11, 2021
@rudy2steiner
Copy link
Author

Currently, Data Mime is connection not message level which will double connections if using different data MIME types.
Object structure isn't a general solution for this problem.

On production environment, we use the first solution as a temporary solution, but we should have more elegant solution.

  • Supporting messaging level data MIME feature is a perfect solution in the future.
  • Supporting hints on client and server also benefits for solving my problem.

Anyway, Supporting hints on client and server is necessary.

I'll try to simplify my changes.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 12, 2021

Currently, Data Mime is connection not message level which will double connections if using different data MIME types.

Keep in mind there is this https://github.com/rsocket/rsocket/blob/master/Extensions/PerStreamDataMimeTypesDefinition.md which we need to add support for. Once that's implemented, doing the same via hints would amount to a workaround, so I think we should create a ticket to support per-stream/request MIME types.

@rudy2steiner
Copy link
Author

rudy2steiner commented Jan 12, 2021

Is there any one has worked on Stream level data MIME, I'd like try if no ?

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 12, 2021

I'd rather focus on support for per-request data MIME types first and close this. I've created the issue #26379. You can give it a try if you want. I don't have a good sense if it would work out well for a contribution.

@rstoyanchev rstoyanchev added status: superseded An issue that has been superseded by another and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 12, 2021
@rudy2steiner
Copy link
Author

Ok, we could polish this feature in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging Issues in messaging modules (jms, messaging) status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants