MONGOSH-296 - Bulk API

[aherlihy]

No description provided.

[mcasimir]

Looks great. Left some sneaky comment, there are 3 you may want to check before merging:

• is it ok to log operations?
• set this._executed in the same context when is read
• there is an !== null that would pass with undefined, is that ok?

[mcasimir]

Maybe is better to add the type for _batchCounts, _batches, _innerBulk, just to make clear what those variables are for, and establish a contract with the different service provider implementations .. and also to get TS to complain a bit more 😄.

[mcasimir]

for example would be great to have a type BatchCounters

[aherlihy]

Ah yes, so for _innerBulk it would be a node driver type but the shell-api class can't import that directly. So instead you are saying we should define interfaces in the service-provider-core that indicate what the innerBulk/batches types should be? I can definitely do that

[mcasimir]

do we need to keep track of the batches in a class property or we should rather have a _getAllBatches method so we always use the service provider state as source of truth?

[aherlihy]

You can only call getOperations after a bulk op is executed, but the bulk op class for the driver deletes the batches attribute after it's executed. So we need to save the batches locally right before we execute. Just a difference from driver to shell api.

[mcasimir]

to be precise in order to avoid race conditions in scripts this._executed should be set right here in the same "synchronous" context where is checked, or in any case before calling any async operation.

[aherlihy]

We can get rid of this entirely because the driver will throw the same error. Or we could just set it to false in the constructor?

[aherlihy]

I removed the error check but kept it it for modifying the batch count - it wouldn't make sense if calling execute twice changed the batch count. Let me know if you think there's a problem with that

[mcasimir]

I think having this._executed there makes sense. my point was that we should not wait to set it to true though:

 async execute(writeConcern?: WriteConcern): Promise<BulkWriteResult> {
    if (this._executed) {
      throw new MongoshInvalidInputError('A bulk operation cannot be re-executed');
    }
    this._executed = true;
   ...

which is different from:

    if (this._executed) {
      throw new MongoshInvalidInputError('A bulk operation cannot be re-executed');
    }

    await doSomething();  // <--

    this._executed = true;

[aherlihy]

But shouldn't this._executed only be set true if the driver successfully executes the bulk op? That's why it's after the await