Skip to content

Sm/qa-jsforce-3-bulk#843

Merged
cristiand391 merged 41 commits intomainfrom
sm/qa-jsforce-3-bulk
Mar 21, 2024
Merged

Sm/qa-jsforce-3-bulk#843
cristiand391 merged 41 commits intomainfrom
sm/qa-jsforce-3-bulk

Conversation

@mshanemc
Copy link
Copy Markdown
Contributor

@mshanemc mshanemc commented Feb 27, 2024

originally created to QA for @W-14996843@

What does this PR do?

  • continued refactoring work for jsforce3 (esp async query to not return results or check even once)
  • use jsforce3 bulk2 polling events
  • allow --use-tooling-api with --bulk on query (Tooling API and Bulk API should not be mutually exclusive for data queries forcedotcom/cli#2667)
  • reorganize the reporters, mostly using functions. If I created a bug, it's somewhere in there. split them into different files, along with their tests
  • more UT for reporter logic
  • 1 nut fix where bulk delete resume human output wasn't being tested
  • drop support for "sfdx" and ":" in help/message output. only sf and spaces
  • improve the perf and reduce API call usages for monitoring bulk jobs
  • improve the error result if you send an uparseable csv file
  • improve the output of the bulk job event updates
  • remove the unused chai-as-promised devDep

Job a07EE00001l5rHsYAI Status Job Complete Records processed 500 Records failed 0. =>
Job a07EE00001l5rHsYAI | Status Job Complete | Records processed 500 | Records failed 0.

there are some // ts-expect-error because jsforce-node.Connection isn't exactly identical to sfdx-core.Connection that extends jsforce v2

one obvious difference is that they have a different implementation of conn.bulk/.bulk2 but there are others about private members (ex: sfdx-core inserts its own logger)

When jsforce-node is rolled out in sfdx-core and sfCommand those will start failing, hopefully, and we can remove them

@W-15141145@

}
};

export const waitOrTimeout = async (job: IngestJobV2<Schema, IngestOperation>, wait: number): Promise<void> => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

jsforce now handles the polling/timeout logic and emits events, so this is removed

);
if (!this.jsonEnabled()) {
displayResults({ ...queryResult }, flags['result-format'] as FormatTypes);
displayResults({ ...queryResult }, flags['result-format']);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

uses the new option flag that types work correctly with

connection.bulk2.pollTimeout = timeout.milliseconds ?? Duration.minutes(5).milliseconds;
let res: Record[];
if (timeout.milliseconds === 0) {
const job = new QueryJobV2(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this would sometimes have results if the query finished quickly. This way it does what the user asked (open and don't be checking) and returns sooner.

try {
res = (await connection.bulk2.query(query, allRows ? { scanAll: true } : {})) ?? [];
return transformBulkResults(res, query);
connection.bulk2.pollTimeout = timeout.milliseconds ?? Duration.minutes(5).milliseconds;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

now handling the unintentional timeout


public static readonly flags = {
'target-org': { ...optionalOrgFlagWithDeprecations, summary: queryMessages.getMessage('flags.targetOrg.summary') },
'target-org': optionalOrgFlagWithDeprecations,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

use the standard summary!

stdout
)
).to.be.true;
expect(stdout).to.match(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

provides nicer output than expected false to be true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's handy to have on of these for QA so I left it in.

@mshanemc mshanemc marked this pull request as ready for review March 1, 2024 22:48
@mshanemc mshanemc requested a review from a team as a code owner March 1, 2024 22:48
// eslint-disable-next-line no-console
console.error(i);
return i;
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this isn't used in the plugin.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'll take it out. I probably need to put it in kit.

@cristiand391
Copy link
Copy Markdown
Member

cristiand391 commented Mar 20, 2024

QA notes:

data query
✅ nested fields are properly rendered
✅ csv reporter outputs valid csv, tested by building csv for record deletion
✅ json reporter
❌ respects wait time with bulk query

--wait is specified in minutes, no matter what value I provide in debug logs the query polling uses the default 30_000ms (30s)

JSFORCE_LOG_LEVEL=DEBUG ./bin/run.js data query --query "SELECT Id, Name,FROM Account" --bulk --wait 10

Screenshot 2024-03-20 at 17 21 05

❌ can do a bulk query on tooling objects
At first I thought it worked when querying ApexClass but that's available outside tooling, after reading the docs linked in the GH discussion I see this requires adding support for the tooling endpoint to do bulk queries on jsforce, see:
https://developer.salesforce.com/docs/atlas.en-us.api_tooling.meta/api_tooling/tooling_api_objects_metadatacomponentdependency_bulk2_usage.htm

IMO we shouldn't implement that yet, mostly because:

  1. only MetadataComponentDependency seems to support tooling bulk queries
  2. it's been in beta for a few years and doesn't seem to be a priority (see group info: https://trailhead.salesforce.com/trailblazer-community/groups/0F93A00000020Vu?tab=discussion&sort=LAST_MODIFIED_DATE_DESC)

✅ bad soql query still shows error from API on --bulk
--wait 0 or --async query, can do query resume and see a 1000ms timeout instead of 0.

data upsert bulk
❌ no update on successful/unprocessed/failed records
also couldn't find the polling logs when running this:

JSFORCE_LOG_LEVEL=DEBUG ./bin/dev.js data upsert bulk --file test/test-files/data-project/data/bulkUpsertLarge.csv --external-id Id --sobject Account --wait 10

seems using the bulk2 from conn instead of the one from jsforce-node, that explains the missing events for the indicator

data delete bulk
❌ no update on successful/unprocessed/failed records

same as upsert bulk

@mshanemc
Copy link
Copy Markdown
Contributor Author

IMO we shouldn't implement that yet,

Yeah, I agree. I'll take that stuff out.

@cristiand391
Copy link
Copy Markdown
Member

cristiand391 commented Mar 21, 2024

QA update:

❌ respects wait time with bulk query

✅ fixed, passed multiple values in min and jsforce gets those in ms

❌ can do a bulk query on tooling objects

--bulk and --use-tooling-api are exclusive again

data upsert bulk

❌ no update on successful/unprocessed/failed records

✅ fixed, command catches inProgress events and updates the spinner on each poll

❌ respects wait time
testes multiple values with --wait, jsforce ends up with a big timeout on polling and only exits on finish or Ctrl-c.

JSFORCE_LOG_LEVEL=DEBUG ./bin/run.js data upsert bulk --file test/test-files/data-project/data/bulkUpsertLarge.csv --external-id Id --sobject Account --wait 10
Screenshot 2024-03-21 at 11 59 18

JSFORCE_LOG_LEVEL=DEBUG ./bin/run.js data upsert bulk --file test/test-files/data-project/data/bulkUpsertLarge.csv --external-id Id --sobject Account --wait 1
Screenshot 2024-03-21 at 12 03 59

spinner status after 2m
Screenshot 2024-03-21 at 12 05 17

data delete bulk

❌ no update on successful/unprocessed/failed records

✅ fixed

❌ respects wait time

same as upsert bulk

Screenshot 2024-03-21 at 12 14 05

@cristiand391
Copy link
Copy Markdown
Member

cristiand391 commented Mar 21, 2024

QA notes:

data delete bulk and data upsert bulk:

❌ respects wait time

✅ fixed
✅ print message if to org open in org if a record failed
✅ exits after 1m on --wait 1 and big bulk job
✅ exit code !=0 on record failures

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants