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

Update pattern for multiple mview suffixes #1693

Merged

Conversation

Swiddis
Copy link
Collaborator

@Swiddis Swiddis commented Apr 15, 2024

Description

Update: Now I'm going with the proposed fix that breaks BWC briefly but doesn't introduce a bug (technically the prefix issue still exists but is much less likely/more work-around-able with two underscores), I think it's better to rip off the band-aid now. I've propagated this to all the merged integrations but it still needs to make its way to the in-progress ones. Including the old description below for context.

Old Description

Updates the flint materialized view pattern with a wildcard that supports multiple suffixes. This ends up being a bit less of a change than expected if we don't enforce any specific separation between the table and the mview suffixes (but by convention there should probably be an underscore separator). In particular:

  • This pattern is backwards-compatible with the old single-mview index, requiring no changes to existing integrations.
  • This pattern is compatible with the search pattern we use to find integrations for a data source.

It's slightly inelegant to allow appending directly to the table name without a separator, but we gain a much smaller delta in exchange.

One issue: there will be a conflict if one table name is a prefix of another: this pattern will lead to integrations with the table name example_table being interfered with by data targeting integrations with the table name example_table_1 or example_table_2, since _1 and _2 would be caught in the wildcard. I'm not sure if there's really a nice way to avoid this while having the guarantees above, I also considered a pattern with a double-underscore separator like {table}__*_mview to have more separation but this breaks backwards compatibility and is messy for integrations with only one view (they'd need to resort to a name with ___mview). Although maybe __* would be more flexible and allows a (relatively) nice {table_name}__ or {table_name}__mview name?

Issues Resolved

Unblocks #1691.

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Swiddis Swiddis added the integrations Used to denote items related to the Integrations project label Apr 15, 2024
@Swiddis Swiddis added enhancement New feature or request backport 2.x labels Apr 15, 2024
@Swiddis Swiddis marked this pull request as draft April 15, 2024 23:20
@Swiddis Swiddis marked this pull request as ready for review April 15, 2024 23:33
@Swiddis
Copy link
Collaborator Author

Swiddis commented Apr 15, 2024

Removing auto-backport label because timing is important for avoiding merge conflicts, will add it when the dependency backports are merged

@Swiddis Swiddis merged commit 72283be into opensearch-project:main Apr 15, 2024
20 of 25 checks passed
@Swiddis Swiddis deleted the feature/integ_multi_mview_pattern branch April 15, 2024 23:54
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 15, 2024
* Update pattern for multiple mview suffixes

Signed-off-by: Simeon Widdis <[email protected]>

* Update field filter to new format

Signed-off-by: Simeon Widdis <[email protected]>

* Update all mviews for new pattern

Signed-off-by: Simeon Widdis <[email protected]>

---------

Signed-off-by: Simeon Widdis <[email protected]>
(cherry picked from commit 72283be)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
YANG-DB pushed a commit that referenced this pull request Apr 15, 2024
* Update pattern for multiple mview suffixes



* Update field filter to new format



* Update all mviews for new pattern



---------


(cherry picked from commit 72283be)

Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
amsiglan pushed a commit to amsiglan/dashboards-observability that referenced this pull request Jun 7, 2024
* Update pattern for multiple mview suffixes

Signed-off-by: Simeon Widdis <[email protected]>

* Update field filter to new format

Signed-off-by: Simeon Widdis <[email protected]>

* Update all mviews for new pattern

Signed-off-by: Simeon Widdis <[email protected]>

---------

Signed-off-by: Simeon Widdis <[email protected]>
(cherry picked from commit 72283be)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request integrations Used to denote items related to the Integrations project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants