-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix deperacation log backward compatibility issue. #60561
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
Conversation
|
Pinging @elastic/es-core-infra (:Core/Infra/Logging) |
|
@howardhuanghua The date part is optional as per https://tools.ietf.org/html/rfc7234#section-5.5 |
|
@howardhuanghua I can see how this can fail in 6.4. Can you please include high level/rest reproduce steps ? |
|
Hi @pgomulka thanks for looking this issue. The reproduce steps should be simple:
|
|
@howardhuanghua to reproduce there has to be a warning emitted on both upgraded and old parts of the cluster. https://github.com/elastic/elasticsearch/blob/6.4/server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java#L491 If you look closely the stracktrace that you pasted on 1 comment actually emits: the stactrace lines fits @jasontedor do you think we should merge this PR in 6.8? Or maybe we should only send a timestamp when serialising ThreadContext to 6.x node (from 6.8) |
|
@pgomulka I would like to confirm that the warning header is always added based on the follow logic? elasticsearch/server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java Line 234 in dfd91e0
Since 6.8 removed the timestamp field from waning header, then the older (6.4) node could not parse the incoming depercation log. That means once user has depercation usage on 6.8, and if cluster also has older version node, then it would be impacted? |
|
Hi @pgomulka would you please help to check that if we could merge this fix to 6.8? It's easy to hit this issue in upgrading case in our production env. |
|
@howardhuanghua I discussed this internally and we decided that even thought this is indeed a valid bug report we won't be fixing it. |
Issue
In our production environment, we try to rolling restart nodes to update from 6.4.3 to 6.8.2, after upgrading one node to 6.8.2, we found lots of follow query fetch exceptions on this upgraded node:
We also could see several above exceptions on other data node, but not that much as this upgraded node.
Analysis
After try catch the
StringIndexOutOfBoundsExceptionexception string on 6.4.3 nodes, we found the exception string is:299 Elasticsearch-6.8.2-bf84795 "'y' year should be replaced with 'u'. Use 'y' for year-of-era. Prefix your date format with '8' to use the new specifier."The above string is generated on 6.8.2 node, it's because we used joda date formatter string "yyyy-MM-dd HH:mm:ss" in query request body. The deprecation log will be sent to target data node in query phase response header:
elasticsearch/server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java
Line 234 in dfd91e0
However, 6.4.3 version expect double strings in warning formatter, it has date formatter in the end:
elasticsearch/server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java
Line 165 in fe40335
This caused 6.4.3 cannot parse the incoming 6.8.2 deprecation log,
penultimateQuoteandfirstQuoteare equal:elasticsearch/server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java
Line 271 in fe40335
This will cause all the queries/fetches with depercation logging failed on 6.4.3 data nodes that received the 6.8.2 depercation log. As the
responseHeadersofThreadContextis common in threadpool, andextractWarningValueFromWarningHeadermethod always be called before 6.8.2 version.Solution
To avoid date format performance issue in #37622, add fake date time string at the end of warning headers to bwc with versions before 6.8.