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

ISPN-9604 Support to run converters in the user requested format #6334

Merged

Conversation

gustavocoding
Copy link

* in the request format rather than the format specified in {@link #format()}. The request format is defined as the MediaType
* that a cache was previously decorated with {@link org.infinispan.AdvancedCache#withMediaType(String, String)}.
*/
default boolean useRequestFormat() {
Copy link
Member

Choose a reason for hiding this comment

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

The converter and listener are already strongly coupled, e.g. you can't use KeyValueVersionConverter without knowing the format of the byte[] it produces. IMO it would be clearer if the listeners with converters ignored the request format completely and relied only on the converter's input/output format -- although I'm not sure how that would work for NearCache/Spark.

Copy link
Author

Choose a reason for hiding this comment

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

it would be clearer if the listeners with converters ignored the request format completely and relied only on the converter's input/output format

It would mean the converter itself would need to get hold of the transcoders and convert the data to the request format. This seems a bit complicated, as converters can be deployed by the user, and getting hold of component registry, obtaining transcoders, injecting, etc should not be exposed to the user IMO.

Copy link
Member

Choose a reason for hiding this comment

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

I was suggesting that converters should ignore the request format, not convert to it. The listener and the converter already have to agree the "outer" format, which is not necessarily the request format, it should be easy to agree on the "inner" format as well without involving the request format at all.

Copy link
Author

Choose a reason for hiding this comment

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

So far the solutions I though were:

  • Assume converters produce custom formats that will not be transcoded to the request format, and let the clients do it all. This will bloat clients with lots of deps to support all formats, things that the server can do it already 👎 👎
  • Create a special KeyValueConverter that has access to the request format and can perform the conversions by calling the appropriate transcoders. Downside: handling of this special converter everywhere 👎
  • This PR, "flag" to allow the converter to process data in the request format already 👍
  • Have a way to send Keys and Values in an event, so I don't even need a converter to glue a Key and Value in a single byte[]. 👍 👍 👍 Most clean solution, but I have a feeling it's not for a micro version, as we'd need to provide a new Converter interface

Copy link
Author

Choose a reason for hiding this comment

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

it should be easy to agree on the "inner" format as well without involving the request format at all.

Could you give an example?

Copy link
Member

Choose a reason for hiding this comment

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

[...] I actually tried to create a parameterised converter that receives the media type for keys and values, but hit a wall with useRaw=true, as it is supposed to have precedence over format(), and also affect the way parameters are sent [...]

IMO if useRawData and format() overlap, it should be impossible to use both, but of course there's no way to not use format()...

It's sure one way of doing it, but that would impose that the converter produces something in the exact format the client expects. There are actually plenty of tests where a converter deployed in the server produces a random POJO, and the client receives a jboss serialized byte[].

Sure, there's a bit of magic to translate from jboss-marshalled objects to and from POJOs, but because the same magic happens on the client, the client listener ends up seeing exactly the POJO that the converter generated. So I don't think that invalidates my point.

Actually, I'd rather not touch any of this stuff in a micro version. This PR was a 'conservative' way of supporting the Spark use case, if it helps I can mark the method as 'dangerous/experimental/advanced' otherwise we can just postpone the work to the next version.

@Experimental works for me, I know it's the least intrusive fix, but it seems like a hack on top of a hack, so I really don't want to commit to it as an API. And if you can, please throw an exception when the converter uses it together with a custom format() and/or useRawData=true.

Copy link
Author

Choose a reason for hiding this comment

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

IMO if useRawData and format() overlap, it should be impossible to use both, but of course there's no way to not use format()...

useRawData takes precedence over format() currently for backwards compatibility reasons, but it really should be removed, created https://issues.jboss.org/browse/ISPN-9634.

so I really don't want to commit to it as an API.

+1. The whole problem stems from the lack of proper API to receive keys and values from events, this should be addressed https://issues.jboss.org/browse/ISPN-9635. Once it's done, KeyValueVersionConverter (and the changes in this PR) can be gone and Spark and Near Cache don't need to use an ad-hoc protocol inside the Hot Rod protocol to see the event value

Copy link
Author

Choose a reason for hiding this comment

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

@danberindei Updated, marking it as experimental

Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking: near cache already doesn't need ISPN-9635, because eager mode is no longer supported ;)

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, I saw that the converter was deprecated since years :)
Still, remote JCache needs to receive values and keys.

@danberindei danberindei merged commit 7e2c1a6 into infinispan:master Oct 26, 2018
@danberindei
Copy link
Member

Integrated, thanks Gustavo!

@gustavocoding gustavocoding deleted the ISPN-9604/converter_req_format branch October 26, 2019 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants