Skip to content

Fixes issue #7631 with wrong archiver lookup #7748

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

Closed

Conversation

PADRESH
Copy link

@PADRESH PADRESH commented May 9, 2025

Calling the GetVisibilityArchiver and GetHistoryArchiver functions causes errors when the internal frontend is enabled. We should either check the current configuration or simply call these functions with the InternalFrontendService parameter if an error occurs.

What changed?

Added fallback logic for archiver retrieval in namespace_handler.go and workflow_handler.go to handle cases when the initial call fails.

Why?

When the internal frontend is enabled, all visibility and history containers are registered under the InternalFrontendService key. However, some calls in namespace_handler.go and workflow_handler.go still attempt to retrieve archivers using the FrontendService key.

How did you test it?

  • [ v] built
  • [v ] run locally and tested manually
  • [v ] covered by existing tests
  • added new unit test(s)
  • added new functional test(s)

Potential risks

If the archival configuration is incorrect, the system takes additional time to fail/recognize the error.

Calling the GetVisibilityArchiver and GetHistoryArchiver functions causes errors when the internal frontend is enabled. We should either check the current configuration or simply call these functions with the InternalFrontendService parameter if an error occurs.
@PADRESH PADRESH requested a review from a team as a code owner May 9, 2025 18:54
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@yycptt
Copy link
Member

yycptt commented May 9, 2025

Thanks for the contribution @PADRESH!

It looks like the issue is caused by namespaceHandler and workflowHandler on frontend hardcoded to use primitives.FrontendService when getting the archiver implementation.

Instead of adding fallback logic, I think we should remove those hardcoded service name and plumb through the actual serviceName to those two components. Similar to what RegisterBootstrapContainer is doing.
primitives.ServiceName type is already available in the fx graph and in HandlerProvider in service/frontend/fx.go, so only NewWorkflowHandler and newNamespaceHandler needs to be updated to take in an additional ServiceName argument.

cc @dnr who has more context on internal frontend.

Calling the GetVisibilityArchiver and GetHistoryArchiver functions causes errors when the internal frontend is enabled. Use serviceName from fx.
…ver_lookup

# Conflicts:
#	service/frontend/namespace_handler.go
#	service/frontend/workflow_handler.go
@PADRESH
Copy link
Author

PADRESH commented May 10, 2025

@yycptt, thank you for the suggestions. I've committed the changes and tested them locally (kubernetes).

@PADRESH
Copy link
Author

PADRESH commented May 13, 2025

@yycptt, @dnr What do you think about my last commit? What else can I do to promote the request?

@dnr
Copy link
Member

dnr commented May 14, 2025

This is okay for what it is, but the problem is really that the archival provider/bootstrap/fx stuff is kind of a mess. There's no reason that it should take a service name at all, each fx app gets its own archiverProvider so it will only ever be called with one service name. The "Bootstrap" structs should just be parameter lists or fx.In structs. If you'd like to tackle refactoring this all, that would be even better than this fix, since it would remove a lot of unnecessary code.

Though I think we can accept this in the short term if it fixes the problem.

I ran CI for this PR... note the failures and please fix.

@dnr
Copy link
Member

dnr commented May 14, 2025

This is okay for what it is, but the problem is really that the archival provider/bootstrap/fx stuff is kind of a mess. There's no reason that it should take a service name at all, each fx app gets its own archiverProvider so it will only ever be called with one service name. The "Bootstrap" structs should just be parameter lists or fx.In structs.

I just tried using an AI tool to do this refactoring and came up with #7766

@PADRESH Could you check to see if that one solves the problem?

@PADRESH
Copy link
Author

PADRESH commented May 31, 2025

@dnr Yes. Problem solved.

@dnr dnr closed this Jun 2, 2025
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.

5 participants