[Connector API] Add metadata to sync job stats endpoint #109927
Conversation
jedrazb
added
>non-issue
:SearchOrg/Extract&Transform
|
Documentation preview: |
elasticsearchmachine
added
the
Team:SearchOrg
|
Pinging @elastic/ent-search-eng (Team:SearchOrg) |
jedrazb
requested review from
efegurkan,
seanstory,
artem-shelkovnikov and
navarone-feekery
...ication/connector/syncjob/action/UpdateConnectorSyncJobIngestionStatsActionRequestTests.java
Show resolved
Hide resolved
| @@ -184,6 +184,36 @@ setup: | |||
| - match: { indexed_document_volume: 1000 } | |||
| - match: { last_seen: 2023-12-04T08:45:50.567149Z } | |||
|
|
|||
| --- | |||
| "Update the ingestion stats for a connector sync job - with optional metadata": | |||
There was a problem hiding this comment.
Should we also have a test for when metadata is in an unexpected type? (like a string or int)
I don't know if this is necessary but I am curious what happens if we give the wrong value type
There was a problem hiding this comment.
Yup, added tests. For str or int we would return 400 as a parsing exception (parser expects an obejct)
|
@elasticmachine merge upstream |
| @@ -57,6 +57,7 @@ public static class Request extends ConnectorSyncJobActionRequest implements ToX | |||
| private final Long indexedDocumentVolume; | |||
| private final Long totalDocumentCount; | |||
| private final Instant lastSeen; | |||
| private final Object metadata; | |||
There was a problem hiding this comment.
Is using Object a standard thing to do in our APIs for "JSON Objects?" Feels too generic, since Object is the base class of all things Java. I would have expected Map<String, String> or JsonObject or something less vague. But that might just be me not being familiar with established ES patterns.
There was a problem hiding this comment.
Defining metadata type as Map<String, String> would be too strict as it doesn't allow for nested maps, which in theory should be allowed by the connector protocol. The connector protocol vaguely defines metadata as a mapping, so agreed we could reasonably represent it as Map<String, Object>.
I used Object since initially I though metadata could be anything (map, int, string) but later on it was clarified to be a map. Will adapt that 👍
| @@ -66,6 +67,7 @@ public Request(StreamInput in) throws IOException { | |||
| this.indexedDocumentVolume = in.readLong(); | |||
| this.totalDocumentCount = in.readOptionalLong(); | |||
| this.lastSeen = in.readOptionalInstant(); | |||
| this.metadata = in.readGenericValue(); | |||
There was a problem hiding this comment.
A lot of the usages I see for in.readGenericValue() us casting. Like
this.metadata = (Map<String, Object>) in.readGenericValue();
There was a problem hiding this comment.
or even better in.readGenericMap()!
|
@elasticmachine merge upstream |
They should test the same path - parsing of the request payload. Yes, we could add explicit parsing tests as unit tests as well 👍 I like defining those |
|
@seanstory I adapted the PR, have a look! As an example I added unit tests that can test the parsing logic, took me a while so IMO we are fine using just YAML tests for that |
|
@elasticmachine merge upstream |
Changes
Extends existing set sync stats api with optional request body parameter
metadata.Connector-specific metadata should be updated along with sync stats, see: connector service reference code.
Endpoint documentation is updated.
Validation