Log server version on sink task startup #561
Conversation
|
|
||
| <properties> | ||
| <es.version>7.9.3</es.version> | ||
| <es.version>7.0.1</es.version> |
There was a problem hiding this comment.
We reverted the version upgrade from here: https://github.com/confluentinc/kafka-connect-elasticsearch/pull/557/files until we can confirm that it will not break compatibility
There was a problem hiding this comment.
Let's revert on a separate PR that explicitly says which change (commit) get reverted.
kkonstantine
left a comment
kkonstantine
left a comment
There was a problem hiding this comment.
@ilanjiR I'd recommend reverting on a separate PR to make changes more clear. Also, let's mention on the title that the logging change is MINOR:
|
|
||
| <properties> | ||
| <es.version>7.9.3</es.version> | ||
| <es.version>7.0.1</es.version> |
There was a problem hiding this comment.
Let's revert on a separate PR that explicitly says which change (commit) get reverted.
| try { | ||
| response = highLevelClient.info(RequestOptions.DEFAULT); | ||
| esVersionNumber = response.getVersion().toString(); | ||
| } catch (IOException | ElasticsearchStatusException e) { |
There was a problem hiding this comment.
we should catch Exception e and also log the exception. This way getting version is an optional thing and we should start the connector regardless
src/main/java/io/confluent/connect/elasticsearch/ElasticsearchSinkTask.java
Outdated
Show resolved
Hide resolved
| this.client = client != null ? client : new ElasticsearchClient(config, reporter); | ||
|
|
||
| log.info("Started ElasticsearchSinkTask."); | ||
| log.info("Started ElasticsearchSinkTask. Connecting to ES server version: {}", |
There was a problem hiding this comment.
in the future scapping logs wont work. We should file another enhancement ticket to log this version as a metric this way we can plot it in datadog
There was a problem hiding this comment.
| response = highLevelClient.info(RequestOptions.DEFAULT); | ||
| esVersionNumber = response.getVersion().toString(); | ||
| } catch (Exception e) { | ||
| log.info("Failed to get ES server version", e); |
There was a problem hiding this comment.
error is quite strict for this soft information.
log.warn seems like a good compromise
There was a problem hiding this comment.
Also, if we use the high level client only to get the version, should we close it (and discard it) here?
What's the life cycle of this client? It might utilized threads, etc.
There was a problem hiding this comment.
This is even better idea. Agree on removing the instance level field and closing it here and removing the call from stop.
src/main/java/io/confluent/connect/elasticsearch/ElasticsearchSinkTask.java
Show resolved
Hide resolved
src/main/java/io/confluent/connect/elasticsearch/ElasticsearchSinkTask.java
Outdated
Show resolved
Hide resolved
| try { | ||
| highLevelClient.close(); | ||
| } catch (Exception e) { | ||
| log.error("Failed to close high level client"); |
There was a problem hiding this comment.
also put this in finally of the above try?
There was a problem hiding this comment.
I was thinking that leaving the client open could be potentially problematic over the long life span of a cluster
|
@ilanjiR LGTM. just resolve the last minor comment before merge |
|
Will merge on green build |
| try { | ||
| highLevelClient.close(); | ||
| } catch (Exception e) { | ||
| log.warn("Failed to close high level client"); |
There was a problem hiding this comment.
please add the exception as last argument
Problem
We need to know what version of ES server our customers are running in cloud so we can determine if it's safe to upgrade
Solution
Log ES server version on sink task startup
Does this solution apply anywhere else?
If yes, where?
Test Strategy
Testing done:
Release Plan