Port AOSS propagation race fix and LDD load hardening from registry-common#291#91
Port AOSS propagation race fix and LDD load hardening from registry-common#291#91jordanpadams wants to merge 1 commit into
Conversation
…ommon#291 Sister PR to NASA-PDS/registry-common#291. Applies all applicable changes to the registry-loader common/ module. Changes where the registry-loader differs from registry-common (structured metadata support, dot-separated field paths) are preserved. Changes ported: - ConnectionFactory: add isServerless() default method (returns false) - UseOpensearchSDK2: add isServerless() override - Request: add MGet.setRefresh(boolean) and Search.buildListLddsNoCache(String) - MGetImpl (aws): implement setRefresh() via craftsman.refresh() - GetImpl (es): implement setRefresh() as no-op (SDK1 ES path) - SearchImpl (aws): implement buildListLddsNoCache() with requestCache=false - SearchImpl (es): implement buildListLddsNoCache() (cache disabled at index level) - DataDictionaryDao: add getLddInfoNoCache(), merge getDataTypesWithRefresh() into getDataTypes(ids, forceRefresh) overload; preserves structured-metadata id mapping - JsonLddLoader: in-memory loadedThisRun cache; SearchIndexWait for sentinel and mget visibility; guard zero-field LDD sentinel; loadOnly() package-private returning boolean; CaaCallback tracks firstFieldId - SchemaUpdater: store conFact; wrap getDataTypes in SearchIndexWait.untilVisible on AOSS; improved DataTypeNotFoundException error message; debug log for already-loaded - SearchIndexWait: new package-private polling utility (ported from registry-common) - AutogenExtractor: skip empty container elements to prevent DataTypeNotFoundException - FileDownloader: include URL in retry error message; demote downloadOnce log to debug - TestSearchIndexWait: 10 unit tests - TestAutogenExtractor: 2 regression tests (field paths use '.' separator per registry-loader structured metadata convention) Not ported: - ClassAttrAssociationParser throw-on-missing-association: registry-loader warns and continues to support structured metadata LDD formats Fixes #81 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
nutjob4life
left a comment
nutjob4life
left a comment
There was a problem hiding this comment.
I'm having déjà vu all over again 😆
I did find 2½ issues this time around; see the interspersed comments and if they can be approached or explained, we're good to go.
| public Get setIndex(String index) { | ||
| this.setIndex(index); | ||
| return this; |
There was a problem hiding this comment.
I get that these three lines were not part of the PR but isn't this just a stack overflow waiting to happen? It's a recursive call to itself with no base case.
| // Wait until the LDD_Info sentinel is visible via search (confirms bulk load completed). | ||
| LddVersions info = SearchIndexWait.untilReady(SearchIndexWait.DEFAULT_WAIT_SECONDS, | ||
| () -> { try { return dao.getLddInfoNoCache(namespace); } catch (IOException e) { throw e; } catch (Exception e) { throw new IOException(e); } }, | ||
| v -> !v.isEmpty(), log, "LDD sentinel for namespace " + namespace); |
There was a problem hiding this comment.
This proves only that some LDD info exists for the namespace. If the namespace already had prior LDDs loaded, this can pass immediately before the newly loaded lddFileName is visible. That undercuts the AOSS race fix.
Maybe?
v -> v.files.contains(lddFileName)or otherwise confirm the newly loaded LDD version/date.
| // no text content and no corresponding LDD attribute entry, so indexing them as | ||
| // an empty-string field would later cause DataTypeNotFoundException during schema | ||
| // resolution. See https://github.com/NASA-PDS/registry-common/issues/293. | ||
| if(fieldValue.isEmpty()) return; |
There was a problem hiding this comment.
FYI, this skips more than just empty container elements like
<empty_container/>but will also skip empty text leaf nodes like
<non_empty_but_skipped_container> </non_empty_but_skipped_container>The comment says it only skips the former.
But maybe that's the correct behavior! If so, the comment should be updated. If not, then the code.
2 participants
🗒️ Summary
Sister PR to NASA-PDS/registry-common#291 — ports all applicable fixes to the registry-loader
common/module. The duplication is intentional while registry-common is being deprecated.Changes ported
AOSS propagation race fixes (#81)
ConnectionFactory: addisServerless()default methodUseOpensearchSDK2: addisServerless()overrideRequest: addMGet.setRefresh(boolean)andSearch.buildListLddsNoCache(String)MGetImpl(aws),GetImpl(es),SearchImpl(aws/es): implement new interface methodsDataDictionaryDao: addgetLddInfoNoCache(); addgetDataTypes(ids, forceRefresh)overload while preserving the structured-metadata id-mapping logic unique to registry-loaderJsonLddLoader: in-memoryloadedThisRuncache to avoid redundant AOSS queries;SearchIndexWaitfor sentinel and mget visibility with 30s timeout; guard against zero-field LDD writing theLDD_Infosentinel;loadOnly()demoted to package-private returning boolean;CaaCallbacknow tracksfirstFieldIdSchemaUpdater: storesconFact; wrapsgetDataTypesinSearchIndexWait.untilVisibleonly on AOSS deployments; improvedDataTypeNotFoundExceptionerror message; debug log for already-loaded LDDSearchIndexWait: new package-private polling utility classBug fixes
AutogenExtractor: skip empty container elements (e.g.<geom:Illumination_Geometry/>) that have no text content — these causedDataTypeNotFoundExceptionduring schema resolution (NASA-PDS/registry-common#293)FileDownloader: include URL in retry error message (was anonymous); demote per-file download log to DEBUGWhat was NOT ported
ClassAttrAssociationParserthrow-on-missing-association: registry-loader intentionally warns and continues here to support structured metadata LDD formats where the association format may differTest differences
TestAutogenExtractoruses.(dot) as the field separator instead of/(slash) because registry-loader usesMetaConstants.ATTR_SEPARATOR = "."for structured metadata support.⚙️ Test data and/or report
TestSearchIndexWait: 10 unit tests — all passTestAutogenExtractor: 2 regression tests (real New Horizons MVIC label) — all passmvn clean install— BUILD SUCCESS (26 tests, 0 failures)♻️ Related issues
Fixes #81
Sister PR: NASA-PDS/registry-common#291
🤓 Reviewer checklist