Skip to content

Add the ability to implement and test custom advanced visibility stores#4871

Merged
rodrigozhou merged 3 commits intotemporalio:mainfrom
aromanovich:custom-adv-visibility
Nov 22, 2023
Merged

Add the ability to implement and test custom advanced visibility stores#4871
rodrigozhou merged 3 commits intotemporalio:mainfrom
aromanovich:custom-adv-visibility

Conversation

@aromanovich
Copy link
Copy Markdown
Contributor

What changed?

Two things:

  • common/persistence/tests/visibility_persistence_suite_test.go renamed to visibility_persistence_suite.go, so that its VisibilityPersistenceSuite could be imported and run by the third-party package;
  • VisibilityStoreFactory interface introduced. The server now accepts customVisibilityStoreFactory and invokes it if visibility store configuration points to CustomDataStoreConfig.

Why?

To be have an ability to implement and test custom advanced visibility stores.

How did you test it?
Ran Temporal tests.
Also built and ran a Temporal server with temporal.WithCustomVisibilityStoreFactory(mypackage.NewMyVisibilityStoreFactory()) option passed.

Potential risks
None.

Is hotfix candidate?
No.

@aromanovich aromanovich requested a review from a team as a code owner September 13, 2023 17:07
@yiminc yiminc requested a review from rodrigozhou October 28, 2023 03:49
@aromanovich
Copy link
Copy Markdown
Contributor Author

@rodrigozhou, does this feature seem acceptable in principle? If yes, I would rebase and resolve conflicts so we could proceed with the review.

@rodrigozhou
Copy link
Copy Markdown
Contributor

@aromanovich Yes, please. This feature sounds nice!

@aromanovich aromanovich force-pushed the custom-adv-visibility branch from 3f0ad69 to fdddc67 Compare November 17, 2023 14:54
@aromanovich
Copy link
Copy Markdown
Contributor Author

@rodrigozhou, done.

I would greatly appreciate if you could also take a look at #5118 — it's a bit related. Merging them would make the life of https://github.com/yandex/temporal-over-ydb a lot easier :)

@aromanovich
Copy link
Copy Markdown
Contributor Author

Gentle bump. Just in case this got lost 🙂

Copy link
Copy Markdown
Contributor

@rodrigozhou rodrigozhou left a comment

Choose a reason for hiding this comment

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

LGTM, just added a nit comment.

Comment on lines 307 to 308
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

visStore and err must be nil if it didn't enter any of those branches, so the last return should work.

@rodrigozhou
Copy link
Copy Markdown
Contributor

@aromanovich
Approved, but it has conflicts with main branch again. Can you rebase it so I can merge please?

@aromanovich aromanovich force-pushed the custom-adv-visibility branch from f35cde7 to cbf54a2 Compare November 22, 2023 17:14
@aromanovich
Copy link
Copy Markdown
Contributor Author

@rodrigozhou Done. Semgrep is not passing, but I'm quite sure it's not due to my rebase :)

@rodrigozhou rodrigozhou merged commit 18c5daf into temporalio:main Nov 22, 2023
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