[Connectors API] Unify timestamp field parsing #104416
Conversation
jedrazb
added
>bug
:EnterpriseSearch/Application
|
Hi @jedrazb, I've created a changelog YAML for you. |
jedrazb
force-pushed
the
fix-connectors-api-timestamp-handling
branch
from
bc72114 to
f91f0e2
Compare
|
Pinging @elastic/ent-search-eng (Team:Enterprise Search) |
There was a problem hiding this comment.
From a functional perspective it looks correct, but after rethinking about the solution I'm not 100% sure, if the fix should happen inside the API, but rather in the client (connectors framework). Explained my rationale in the comment.
Still, feel free to merge it, if you think this solution is the right path forward 👍
| builder.field(LAST_INCREMENTAL_SYNC_SCHEDULED_AT_FIELD.getPreferredName(), lastIncrementalSyncScheduledAt); | ||
| builder.field( | ||
| LAST_INCREMENTAL_SYNC_SCHEDULED_AT_FIELD.getPreferredName(), | ||
| ConnectorUtils.formatInstantToFrameworkString(lastIncrementalSyncScheduledAt) |
There was a problem hiding this comment.
As I'm seeing now formatInstantToFrameworkString I'm wondering, if we should fix it the other way around. As the Connectors API guards everything around the protocol it feels cleaner, if the clients (like the framework) adapt to the API and not the other way around. In theory the connectors API should also be useable for other clients/schedulers.
There was a problem hiding this comment.
I agree that the issue should be fixed at the framework level, and I already filed an enhancement request elastic/connectors#2067 to unify handling (parsing, representing) datetimes in connectors framework.
While this PR doesn't solve the underlying root cause it mitigates the symptoms and allows us move forward with integration. Also, think about the connectors created in older releases, we need to be backward compatible with their representation with timestamps (or prepare a migration). Once the issue is solved in framework we can change the API formatting of timestamps.
There was a problem hiding this comment.
Thanks for the explanation, makes sense!
jedrazb
added
auto-backport-and-merge
>non-issue
and removed
>bug
auto-backport-and-merge
v8.12.1
labels
|
@timgrein @navarone-feekery After discussing this in the team sync, I'm adapting this PR to just address the issue of datetime parsing. Since it's not a bugfix I'm removing the label and not backporting this. Can you review again? |
jedrazb
changed the title
|
This change fixes fixing a unit test bug present in initial release (
This PR is not adding a feature, it's unifying the logic we use to handle timestamps for connectors, therefore I'm backporting this to 8.12 to address the build failures. |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit a1d05d5) # Conflicts: # x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJob.java # x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/syncjob/action/UpdateConnectorSyncJobIngestionStatsAction.java
Changes
datetimeparsing logic withparseInstantfunction applied to all datetime fields from user in connectors APIs - user can provide datetime in any ES-compatible format, the API will take care of making it compatible with connectors protocol