Emit error log if deregistered search attribute in chasm vis task #8715
Conversation
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
I would consider making this a warning since we expect this to happen in certain conditions and it is not an error under our control unless we have a severe bug, but that should be covered in functional tests.
| if err != nil { | ||
| return err | ||
| // INFO: To reach here, either the search attribute has been deregistered before task execution, which is valid behavior, | ||
| // or the upserted search attribute is incorrectly mapped, indicating possible bug in application code. |
There was a problem hiding this comment.
WDYM incorrectly mapped? Do you mean that there's a bug in the frontend code?
There's another cause for this that we discussed earlier. The search attribute mapping may have propagation delays to the history node executing this task.
We already ensure that all search attributes are mappable on the frontend.
There was a problem hiding this comment.
in the case of eager workflows, following executions bypassed frontend validators. error message is more specific now.
awln-temporal
force-pushed
the
chasm-visibility-task-deregistered-attribute
branch
from
5aa17eb to
fdab724
Compare
awln-temporal
deleted the
chasm-visibility-task-deregistered-attribute
branch
What changed?
Emit warn log if deregistered search attribute in chasm vis task.
Original thread: #8662 (comment)
Current State
If user deregisters search attributes while the task is getting processed, current code leads to task failure.
In ES, if the search attribute has been deregistered, upsert SA task execution would still fail when generating the ES Doc and attempting to decode the value.
In the SQL implementation, since these fields are constant and always attached to the NameTypeMap/ClusterMetadata, Upserting these fields would succeed, so there is a discrepancy between these stores.
Why?
Once we have parity between ES and SQL for preallocated columns, we should not throw an error if a search attribute gets deregistered when executing the CHASM vis task and just do a best effort upsert call. User deregistering search attributes after archetype execution is a valid use case.
A valid concern is if the visibility component somehow has an incorrect search attribute (eg. field name). There was an issue the past caught because errors were thrown during translation, which showed search attribute already translated to field name before hitting the translation layer (due to application code bug). However, Error logs should be sufficient to debug.
How did you test it?