Make buffer limit configurable in HeapBufferedConsumerFactory #23970
Conversation
The buffer limit should have been configurable already, but the factory constructor is package private so it is truly configurable only from the org.elasticsearch.client package. Also the HttpAsyncResponseConsumerFactory interface was package private, so it could only be implemented from the org.elasticsearch.client package. Closes elastic#23958
javanna
added
:Clients/Java Low Level REST Client
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.client.consumer.consumer; |
There was a problem hiding this comment.
Double consumer? It looks like only one should be necessary to escape the org.elasticsearch.client package.
There was a problem hiding this comment.
yea I don't know what happened there :)
| public class CustomHeapBufferedConsumerFactoryTests extends RestClientTestCase { | ||
|
|
||
| //Note that this test class should be in a different package than org.elasticsearch.client so we can test that | ||
| //the access modifiers are public rather than package private or protected |
| new HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory(bufferLimit); | ||
| HttpAsyncResponseConsumer<HttpResponse> consumer = consumerFactory.createHttpAsyncResponseConsumer(); | ||
| assertThat(consumer, instanceOf(HeapBufferedAsyncResponseConsumer.class)); | ||
| HeapBufferedAsyncResponseConsumer bufferedAsyncResponseConsumer = (HeapBufferedAsyncResponseConsumer) consumer; |
There was a problem hiding this comment.
This line probably exceeds the 100-column limit. I know that we are actively discussing what to do about that, so I'm fine with either bringing it under the limit or suppressing the file for now.
|
|
||
| public class CustomHeapBufferedConsumerFactoryTests extends RestClientTestCase { | ||
|
|
||
| //Note that this test class should be in a different package than org.elasticsearch.client so we can test that |
There was a problem hiding this comment.
I'd probably assert this with reflection.
There was a problem hiding this comment.
Oh, I like that suggestion! Then the custom package isn't needed.
There was a problem hiding this comment.
Good idea, I pushed a new commit. It addresses the constructor visibility problem. Also the interface was mistakenly package private, but that is not caught by the test with reflection in the same package. Do you guys have ideas on how to achieve that too?
There was a problem hiding this comment.
I think something like this would work:
public void testVisibility() throws ClassNotFoundException {
final Class<?> clazz =
Class.forName("org.elasticsearch.client.HttpAsyncResponseConsumerFactory");
assertThat(clazz.getModifiers() & Modifier.PUBLIC, equalTo(Modifier.PUBLIC));
}
| IllegalAccessException, InvocationTargetException, InstantiationException { | ||
| int bufferLimit = randomIntBetween(1, Integer.MAX_VALUE); | ||
| Class<?> aClass = Class.forName(HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory.class.getName()); | ||
| Constructor<?> constructor = aClass.getConstructor(Integer.TYPE); |
There was a problem hiding this comment.
I'd add a comment that you are using reflection here to make sure that the ctor is public.
| } | ||
|
|
||
| public void testHttpAsyncResponseConsumerFactoryVisibility() throws ClassNotFoundException { | ||
| final Class<?> clazz = Class.forName(HttpAsyncResponseConsumerFactory.class.getName()); |
There was a problem hiding this comment.
If we are going to load the class directly (i.e., via the class itself) there is no need to use Class.forName, you can just say final Class<?> class = HttpAsyncResponseConsumerFactory.class;.
There was a problem hiding this comment.
yep that's silly, I will fix thanks
The buffer limit should have been configurable already, but the factory constructor is package private so it is truly configurable only from the org.elasticsearch.client package. Also the HttpAsyncResponseConsumerFactory interface was package private, so it could only be implemented from the org.elasticsearch.client package. Closes #23958
The buffer limit should have been configurable already, but the factory constructor is package private so it is truly configurable only from the org.elasticsearch.client package. Also the HttpAsyncResponseConsumerFactory interface was package private, so it could only be implemented from the org.elasticsearch.client package. Closes #23958
The buffer limit should have been configurable already, but the factory constructor is package private so it is truly configurable only from the org.elasticsearch.client package. Also the HttpAsyncResponseConsumerFactory interface was package private, so it could only be implemented from the org.elasticsearch.client package.
Closes #23958