Repository navigation
Conversation
Pull fetched each entry with only the aspect types catalog.yaml lists and its entry type requires, so it could not tell an author what it left out. A catalog.yaml written before init listed any aspects pulls a knowledge base document with an empty body and says nothing. Pull now fetches each entry with every aspect (the FULL view), keeps the aspects of the types it knows, and warns once for each aspect type it left out, naming the number of entries and how to pull it. What it stores is unchanged. Each scope names the aspects the service writes beyond the required ones (CatalogSource.systemAspects), which pull leaves out without a warning. For a BigQuery dataset these are bigquery-policy and usage, the aspects BigQuery writes on a table besides the ones its type requires. lookupEntry takes 'FULL' to return every aspect. Bug: b/576454554
feixufeixu
left a comment
feixufeixu
left a comment
There was a problem hiding this comment.
Approving with two required fixes. In production a FULL read returns data only for the aspects an entry's type requires. So this pull stops storing the overview that #560 lists, and the new warning never prints. My sync.ts comment gives the fix, and my mocks.ts comment covers why the tests still pass. The other two comments are optional.
| const res = await this._catalog.lookupEntry(nameParts[1], nameParts[3], entry.name, | ||
| aspectTypes); | ||
| 'FULL'); |
There was a problem hiding this comment.
Required. In production a FULL read gives data only to the aspects the entry type requires. Every other aspect comes back as {}, and _fixEntry drops it at dataplex.ts:451. So pull stores no listed aspect beyond the required ones. A knowledge base document pulls with an empty body again, and the warning below never prints.
I'd keep #560's read here, with [...aspectTypes.keys()] as the last argument, and store its result as before. For the warning, I'd add a CatalogClient method that reads the entry through _get with view: 'FULL'. It returns the distinct aspect types of the entry's keys as project.location.type. It drops the @path part of each key and resolves project numbers with crm.fixProject. A type is left out when it is in that list, the first read did not return it, aspectTypes does not have it, and it is not a system aspect. lookupEntry then goes back to its #560 version. This costs one more read per entry. The change also keeps a listed or required aspect when Resource Manager cannot resolve the project number of its type. Line 82 now drops such an aspect and warns about it. view=ALL would not work. Production returns at most 100 aspects for it, so an entry with more can lose a listed aspect.
| // As Dataplex does: every aspect for 'FULL', else only the aspects of the | ||
| // requested types, none when no type is requested. | ||
| const kept = Object.entries(entry.aspects ?? {}) | ||
| .filter(([, a]) => aspects?.includes(a.aspectType ?? '')); | ||
| .filter(([, a]) => aspects === 'FULL' || aspects?.includes(a.aspectType ?? '')); |
There was a problem hiding this comment.
Required. This mock answers FULL with every aspect and its data. Production gives data only to the required aspects. That is why pull_left_out and init_kb_pull_overview pass here, while a real pull prints no warning and loses a listed overview. If you keep a FULL read through lookupEntry, please make this mock match production: it lists every aspect key on the entry, and only the required aspects carry data. With my sync.ts change instead, this filter goes back to its #560 version, and the mock of the new method returns every aspect type on the entry.
| // BigQuery writes these on a table besides the ones its type requires. | ||
| readonly systemAspects = [ | ||
| 'dataplex-types.global.bigquery-policy', | ||
| 'dataplex-types.global.usage', | ||
| ]; |
There was a problem hiding this comment.
Optional. Dataplex data profile and data quality scans also write aspects on BigQuery tables when they publish results. For example, bigquery-public-data.stackoverflow.posts_questions carries data-profile. Without them in this list, the warning tells the author to pull aspects that push would then send back.
| // BigQuery writes these on a table besides the ones its type requires. | |
| readonly systemAspects = [ | |
| 'dataplex-types.global.bigquery-policy', | |
| 'dataplex-types.global.usage', | |
| ]; | |
| // BigQuery and Dataplex data scans write these on a table besides the ones | |
| // its type requires. | |
| readonly systemAspects = [ | |
| 'dataplex-types.global.bigquery-policy', | |
| 'dataplex-types.global.data-profile', | |
| 'dataplex-types.global.data-quality-scorecard', | |
| 'dataplex-types.global.usage', | |
| ]; |
| // Dataplex keeps an aspect that aspectKeys names and the body leaves out | ||
| // unless deleteMissingAspects is set, so a caller that means to remove one has | ||
| // to send it. | ||
| describe('CatalogClient.lookupEntry', () => { |
There was a problem hiding this comment.
Optional. The comment on lines 249-251 describes the updateEntry tests further down. I'd put this block, or whatever test replaces it after the sync.ts change, above line 249, so the comment sits right above those tests again.
Part 2 of b/576454554. Built on #560; I'll rebase onto
mainonce it merges.Problem
Pull fetched each entry with only the aspect types
catalog.yamllists and its entry type requires, so it could not say what it left out. With acatalog.yamlfrom before #560, a knowledge base document pulls with an empty body and kcmd is silent.Change
lookupEntry(..., 'FULL')), keeps the aspects of the types it knows, and warns once per aspect type it left out:Warning: pull left out aspect type 'X' on N entries; list it under snapshot.aspects in catalog.yaml to pull it.What pull stores is unchanged.
CatalogSource.systemAspects: aspects the service writes beyond the required ones, left out without a warning. BigQuery dataset:bigquery-policyandusage. These are what a sample of 27 real BigQuery tables carry besides the requiredbigquery-table,schemaandstorage.lookupEntrytakes'FULL'for every aspect; a string list still means those types, and nothing still means none.Tests
pull_left_outscenario: two entries with an unlisted aspect and one withoverview, pulled with acatalog.yamlthat lists none. The aspects are not stored, and pull warns once per type with the entry count.pull_bq: a table withusageandbigquery-policypulls without them and with no warning.console.warnand checksassert.warnings.lookupEntrysends the BASIC, CUSTOM or FULL view as asked.usagefrom the BigQuery list failspull_bq; storing every fetched aspect failspull_bqandpull_left_out; removing the warning failspull_left_out.tscclean; scenarios 24, layouts 2, gcp 12, semantic 2068, tool 395, all pass.