Skip to content

Add Document Key Retriever to Buffered Sender#13478

Merged
sarangan12 merged 5 commits intoAzure:masterfrom
sarangan12:DocKeyRetrieverSearchSDK
Jan 30, 2021
Merged

Add Document Key Retriever to Buffered Sender#13478
sarangan12 merged 5 commits intoAzure:masterfrom
sarangan12:DocKeyRetrieverSearchSDK

Conversation

@sarangan12
Copy link
Contributor

This PR Contains the following changes:

  1. Change @hidden to @internal values.
  2. Fix the error code to statusCode.
  3. One new requirement is that in the batch sender before sending in the batch we need to check if there are any duplicates (based on the id retrieval logic supplied by the user) and move them to the next batch. (It is ok if we send a batch that is less than the threshold size)
  4. Change the autoflush to true by default.

@xirzec Please review and approve.

@ghost ghost added the Search label Jan 29, 2021
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good, just a few thoughts about how we can tighten typing and make the prune function not mutate its parameters.


while (counter < batch.length) {
const { __actionType, ...document } = batch[counter];
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have ts-ignore here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do not add this here, I get the following error:

src/searchIndexingBufferedSender.ts:405:45 - error TS2345: Argument of type 'IndexDocumentsAction<T>' is not assignable to parameter of type 'T'.
  'T' could be instantiated with an arbitrary type which could be unrelated to 'IndexDocumentsAction<T>'.

405       const key = this.documentKeyRetriever(document);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. Since merge and delete operations don't need to specify the entire document, IndexDocumentsAction maps to a Partial plus the action property.

Since the action can be a partial document, that means the mapping function might not be getting the whole document. This should still be fine as the partial document must contain the key, but TS doesn't know that, so what it wants is for documentKeyRetriever to look like this:

documentKeyRetriever: (document: T | IndexDocumentsAction<T>) => string

I notice that buffered sender doesn't actually ever take in partial documents, so I think in this case it's safe to avoid it with

const key = this.documentKeyRetriever(document as unknown as T);

Which is still much better than ts-ignore as ts-ignore disables all checking instead of just one type check.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm happy with this as long as we can take out the ts-ignore (see suggestion in comment)


while (counter < batch.length) {
const { __actionType, ...document } = batch[counter];
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. Since merge and delete operations don't need to specify the entire document, IndexDocumentsAction maps to a Partial plus the action property.

Since the action can be a partial document, that means the mapping function might not be getting the whole document. This should still be fine as the partial document must contain the key, but TS doesn't know that, so what it wants is for documentKeyRetriever to look like this:

documentKeyRetriever: (document: T | IndexDocumentsAction<T>) => string

I notice that buffered sender doesn't actually ever take in partial documents, so I think in this case it's safe to avoid it with

const key = this.documentKeyRetriever(document as unknown as T);

Which is still much better than ts-ignore as ts-ignore disables all checking instead of just one type check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants