Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

salesforce Add support for asyncMode in bulk operation #907

Closed
wants to merge 7 commits into from

Conversation

mtuchi
Copy link
Collaborator

@mtuchi mtuchi commented Jan 14, 2025

Summary

Add asyncMode option in bulk operation. If you don't provided this extra asyncMode option, we return all record info to state.data as we currently do.

Fixes #286

Details

  • If asyncMode is true we return batchInfo
  • Formatted the results to { success: true, batches: [], errors: []} if asyncMode: true
  • Add docs for state.data.batches
  • Add integration test when asyncMode: true

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, added the adaptor on marketing website ?
  • Are there any unit tests?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.
  • Have you ticked a box under AI Usage?

@mtuchi mtuchi linked an issue Jan 14, 2025 that may be closed by this pull request
@mtuchi mtuchi changed the title 286 asyncmode salesforce Add support for asyncMode in bulk operation Jan 14, 2025
@mtuchi mtuchi marked this pull request as ready for review January 14, 2025 14:39
@mtuchi mtuchi requested a review from josephjclark January 14, 2025 14:39
if (asyncMode) {
return batch.on('queue', async function (batchInfo) {
console.info('Batch queued. Batch ID:', batchInfo.id);
resolve({ batchInfo });
Copy link
Collaborator

Choose a reason for hiding this comment

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

A promise can only resolve once. If there are multiple queue events, for multiple chunks, then they'll never report back to user code. In other words, we'll only ever report back the first batch here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤔 I have tried running the test with a batch size of 1 and i got the expected results.
Can you tell me more about this ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have tested multiple batches by reducing the batch size to 1 and run integration test that insert two records with the option asynMode: true, I can see that i am getting two batches records after the bulk operation is done.

@@ -66,6 +66,7 @@ import flatten from 'lodash/flatten';
* @property {boolean} [failOnError=false] - Fail the operation on error.
* @property {integer} [pollTimeout=240000] - Polling timeout in milliseconds.
* @property {integer} [pollInterval=6000] - Polling interval in milliseconds.
* @property {boolean} [asynMode=false] - Use asyn mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo

@josephjclark
Copy link
Collaborator

@mtuchi where are we on this? I'm still not comfortable with this promise that returns multiple times.

When you call resolve(), you are saying to the Javascript: "I have finished with my promise, please continue executing". Until then, the runtime is patiently waiting for the promise to end.

In your code, every time the queue event is called, you're trying to resolve the promise again.

There are two really important things here:

  1. A promise can only return once. If you do promise.then() twice, you'll get the same value back twice. Your code is sort of working because it returns the same array twice - it's just that the array itself has changed
  2. Each Operation is a Promise, and our Runtime waits for each Operation to resolve before executing the next one (or moving on to the next workflow step). Your adaptor code is "exiting" the operation after the first queue call, and so our runtime carries on. What happens when the promise resolves a second time? I actually don't know - but we can't trust the order of events that follows next.

I can't merge this as-is, it's too unsafe.

What we actually want to do is resolve the promise after the last queue event has triggered. But we have no way of knowing that, right? jsforce gives us no "queue finished event.

See the issue for a possible solution, although I'm not totally sure I trust that either 🤔

@josephjclark josephjclark marked this pull request as draft January 29, 2025 14:25
@josephjclark
Copy link
Collaborator

promises demo

@mtuchi
Copy link
Collaborator Author

mtuchi commented Jan 29, 2025

Hiya @josephjclark i have forgotten to close this PR, from our last conversation we don't want this implementation and we agreed to put more effort on updating jsforce instead.

@mtuchi mtuchi closed this Jan 30, 2025
@mtuchi mtuchi deleted the 286-asyncmode branch January 30, 2025 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add option to salesforce for asyncMode: true/false on bulk(...)
2 participants