Add batch_size to identify fields in Drip (Actions) #2932
Conversation
packages/destination-actions/src/destinations/drip/identify/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/drip/identify/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/drip/identify/index.ts
Outdated
Show resolved
Hide resolved
| const batchSize = payload[0]?.batch_size ?? 1000 | ||
| const multiStatusResponse = new MultiStatusResponse() | ||
| for (let i = 0; i < payload.length; i += batchSize) { | ||
| const chunk = payload.slice(i, i + batchSize) | ||
| const subs = chunk.map(person) | ||
| const response = await request(`https://api.getdrip.com/v2/${settings.accountId}/subscribers/batches`, { | ||
| method: 'POST', | ||
| json: { batches: [{ subscribers: subs }] } | ||
| }) | ||
| chunk.forEach((_, index) => { | ||
| multiStatusResponse.pushSuccessResponse({ | ||
| status: response.status, | ||
| sent: subs[index], | ||
| body: response.data as JSONLikeObject | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
hi @jacobpmeyer
Segment's pipeline will ensure that the size of the payload array is already chunked to batch_size. There's no need for you to do this in code.
Also, it seems that you are not inspecting the response values from your requests. Shouldn't your responses tell you if each item in the batch request was good or not?
Finally, if any items are not successful, you should update the multiStatusResponse to indicate which items in the batch failed (for example by using setErrorResponseAtIndex)
Take a look at the multistatus docs here: https://github.com/segmentio/action-destinations/blob/main/docs/multistatus.md
Also there are a few implementations in the repo.
There was a problem hiding this comment.
@jacobpmeyer - perhaps we move multistatus out of this PR to a follow-up, and evaluate its merit? I don't think our usage of multistatus will be super natural because we do less validation on batches, and I'd prefer that it didn't block this PR in any case.
There was a problem hiding this comment.
hi @joe-ayoub-segment Thanks for the info regarding segment's handling of the chunking. That definitely simplifies implementation.
I'd originally assumed that batch_size would be handled under the hood, but I wasn't seeing multiple requests in the return for my curl during local testing so I though maybe I needed to handle the logic for the extra requests.
In any case, I went ahead and removed the MultiStatusResponse code based on @stevedrip's comment and after looking over the doc again I'm not sure we would've gotten much value out of it anyway with the current Drip API. So it's essentially just a light cleanup of the code that was already there.
Let me know if what's there now looks okay.
There was a problem hiding this comment.
Hi @jacobpmeyer thanks for the PR.
I left a few comments for you to look at.
Kind regards,
Joe
|
Hi @joe-ayoub-segment. Thank you so much for the quick and thorough feedback. I made edits to the code where you mentioned. Let me or @stevedrip know if there's anything else that you'd like to see from this PR. I'm going to be out of office tomorrow, so additional changes might come from Steve. |
|
Hi @jacobpmeyer PR deployed. Please confirm that you are happy with the change. |
Hey @joe-ayoub-segment. Would I need to wait until next week to remove |
|
hi @jacobpmeyer yes - Tuesday is the next deploy. |
Adding
batch_sizeto theidentifyfieldsTesting