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

Remove infinispan-jboss-marshalling from query #7388

Closed
wants to merge 1 commit into from

Conversation

ryanemerson
Copy link
Contributor

@ryanemerson ryanemerson commented Oct 2, 2019

https://issues.jboss.org/browse/ISPN-10591

Please don't close the Jira as more PRs to come.

@ryanemerson ryanemerson requested review from anistor and gustavocoding and removed request for anistor October 3, 2019 13:08
@ryanemerson ryanemerson added this to the 10.0.0.CR3 milestone Oct 3, 2019
@@ -207,7 +226,8 @@ public void writeObject(ObjectOutput output, QueryDefinition queryDefinition) th
output.writeObject(queryDefinition.queryEngineProvider);
} else {
output.writeBoolean(false);
output.writeObject(queryDefinition.hsQuery);
output.writeObject(queryDefinition.hsQuery.getLuceneQuery());

Choose a reason for hiding this comment

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

What about the other serializable parts of hsQuery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any fields in mind? Looking at the non-transient fields in LuceneHSQuery impl I don't think we need to marshall any of the fields other than the LuceneQuery in order to recreate the state.

Choose a reason for hiding this comment

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

That are other types of hsQuery unfortunately. I am a bit puzzled no test failed by ignoring the serialization of the other stuff, let me check...

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 are other types of hsQuery unfortunately.

True, that's actually partly why I thought that recreating the HSQuery via the LuceneQuery and SearchIntegrator.createHSQuery was sufficient, as the SearchIntegrator only provides createHSQuery(Query, Class<?>...) and createHSQuery(Query, IndexedTypeMap<CustomTypeMetadata>).

Copy link

Choose a reason for hiding this comment

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

// Internal and experimental! Creates a {@link CacheQuery}, filtered according to the given {@link HSQuery}
SearchManagerImpl#getQuery(HSQuery hSearchQuery, IndexedQueryMode queryMode)

That's where it is used. It's an internal stuff, not part of the SearchManager interface

Copy link

Choose a reason for hiding this comment

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

So if someone is using this, it has already been warned :) WDYT @anistor ? Are you happy with not supporting this method from a user's POV, and keep it only working to the internal callers?

@anistor
Copy link
Member

anistor commented Oct 4, 2019

merged. thanks @ryanemerson !

@anistor anistor closed this Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants