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

Revert modification to label of parent context in router #12

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

leonlnj
Copy link
Contributor

@leonlnj leonlnj commented Mar 14, 2023

Description

This reverts the PR which was initially targeted to fix the label of traffic rules

For detailed explanation, refer to Turing PR which revert fiber from 2.1-rc1 to 2.0

Fundamentally, this behaviour occur due to eager and lazy router calling routes with different sequence.

  • eager router call route with parent context, then call strategy
  • lazy router call strategy, then call route with strategy context

Hence this PR reverts Fiber 2.1-rc1, until a solution fixes both metrics.

@leonlnj leonlnj marked this pull request as ready for review March 14, 2023 02:21
@leonlnj leonlnj requested a review from a team March 14, 2023 02:21
@krithika369
Copy link
Contributor

@leonlnj, in the linked Turing PR, the original bug is with Selective Routing strategy + Default traffic rule, correct?

Calls all routes with context passed (contain NO traffic label, as it is pass the label from DefaultTuringRoutingStrategy which logs metric incorrectly)

@leonlnj
Copy link
Contributor Author

leonlnj commented Mar 14, 2023

Selective Routing strategy + Default traffic rule

Selective Routing strategy, yes. Its not default traffic rule, regardless of rule, at the lowest level it will call individual route with DefaultTuringRoutingStrategy

@krithika369
Copy link
Contributor

Selective Routing strategy + Default traffic rule

Selective Routing strategy, yes. Its not default traffic rule, regardless of rule, at the lowest level it will call individual route with DefaultTuringRoutingStrategy

I see.. Got it. Will append this to the internal bug, can try to pick it up over the coming sprints. Thanks.

@leonlnj leonlnj merged commit 6060a60 into gojek:main Mar 14, 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