Skip to content

Check if sniffer was able to determine the version before testing if it's a 2.x#1190

Open
marko-bekhta wants to merge 1 commit intoelastic:mainfrom
marko-bekhta:fix/check-version-not-null-before-testing-it
Open

Check if sniffer was able to determine the version before testing if it's a 2.x#1190
marko-bekhta wants to merge 1 commit intoelastic:mainfrom
marko-bekhta:fix/check-version-not-null-before-testing-it

Conversation

@marko-bekhta
Copy link

Hey 👋🏻 🙂,

I've run into this:

(es_rest_client_sniffer[T#1]) ERROR Sniffer:140 - error while sniffing nodes
java.lang.NullPointerException: Cannot invoke "String.startsWith(String)" because "version" is null
	at co.elastic.clients.transport.rest5_client.low_level.sniffer.ElasticsearchNodesSniffer.readNode(ElasticsearchNodesSniffer.java:255)
	at co.elastic.clients.transport.rest5_client.low_level.sniffer.ElasticsearchNodesSniffer.readHosts(ElasticsearchNodesSniffer.java:123)
	at co.elastic.clients.transport.rest5_client.low_level.sniffer.ElasticsearchNodesSniffer.sniff(ElasticsearchNodesSniffer.java:107)
	at co.elastic.clients.transport.rest5_client.low_level.sniffer.Sniffer.sniff(Sniffer.java:207)
	at co.elastic.clients.transport.rest5_client.low_level.sniffer.Sniffer$Task.run(Sniffer.java:138)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:545)
	at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:328)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:309)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1090)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:614)
	at java.base/java.lang.Thread.run(Thread.java:1474)

while I was looking into Hibernate Search CI failure (that particular test uses different Elasticsearch clients and "backends" including the one which supposedly won't return the version in the nodes response to sniffer...). So I thought I'd open a PR and let you decide if you want to include the fix 😃.
Alternatively, maybe let's just set String version = ""; ?

@cla-checker-service
Copy link

❌ Author of the following commits did not sign a Contributor Agreement:
6ba7f51

Please, read and sign the above mentioned agreement if you want to contribute to this project

@l-trotta
Copy link
Contributor

Hello, thanks for the PR! I'm very curious: how did you manage to get a node info response without the version field? It should be always present in the response (as per the response builder in the server code), and in fact the rest 5 client sniffer code has been copied and updated from the original sniffer in the elasticsearch server code.

@marko-bekhta
Copy link
Author

hi 🙂 👋🏻

how did you manage to get a node info response without the version field?

ahh, so that particular request to get the version is mocked and in one of the test scenarios the version value is removed from it on purpose... That scenario was suppose to imitate the call to the early versions of the opensearch serverless distribution, which at those times was either not returning the version at all or when it was returning something, it wasn't usable 🫣. (I don't have access to that service anymore so can't confirm if they changed it or not)

I debugged the sniffer/client from the server distribution (apache 4 based one) and it does not have this null exception as it ends up parsing the null as "null" (string).
It's probably the change to Jakarta Json from Jackson that introduced that "difference in behavior" between these two clients 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants