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

Explicitly require a OriginSettingClient in ML results iterators #50802

Merged
merged 3 commits into from
Jan 14, 2020

Conversation

davidkyle
Copy link
Member

@davidkyle davidkyle commented Jan 9, 2020

In classes where the client is used directly rather than through a call to executeAsyncWithOrigin explicitly require the client to be OriginSettingClient rather than use the client interface. Basically that means BatchedDocumentsIterator and the classes that use it.

Also remove calls to deprecated ClientHelper.clientWithOrigin() method.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@davidkyle davidkyle changed the title Explicitly require a OriginSettingClient Explicitly require a OriginSettingClient in ML results iterators Jan 9, 2020
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@droberts195
Copy link
Contributor

Please can you also backport this to 7.x rather than just changing master, as it will avoid future backporting pain to avoid unnecessary differences between master and 7.x

@davidkyle
Copy link
Member Author

The unit tests proved quite problematic as OriginSettingClient is a final class and cannot be mocked with this version of mockito. OriginSettingClient is a filter client wrapping a Client so the solution is to create an OriginSettingClient with a mocked client and do the mocking with the wrapped client. Unfortunately that meant rewriting a lot mocking code as the filter client calls client.execute(Action, ...) rather than the specific methods client.search() etc.

In classes where the client is used directly rather than
executeAsyncWithOrigin and remove calls to deprecated ClientHelper.clientWithOrigin() method
@davidkyle davidkyle force-pushed the make-client0-with-origin-explicit branch from 276badd to d12fdb6 Compare January 14, 2020 10:43
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@davidkyle davidkyle merged commit a5b462c into elastic:master Jan 14, 2020
@davidkyle davidkyle deleted the make-client0-with-origin-explicit branch January 14, 2020 14:37
davidkyle added a commit to davidkyle/elasticsearch that referenced this pull request Jan 14, 2020
…stic#50802)

In classes where the client is used directly rather than through a call to 
executeAsyncWithOrigin explicitly require the client to be OriginSettingClient 
rather than using the Client interface. 

Also remove calls to deprecated ClientHelper.clientWithOrigin() method.
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
…stic#50802)

In classes where the client is used directly rather than through a call to 
executeAsyncWithOrigin explicitly require the client to be OriginSettingClient 
rather than using the Client interface. 

Also remove calls to deprecated ClientHelper.clientWithOrigin() method.
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.

4 participants