Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Change sampler param sentinel value from YAML parser #145

Merged
merged 2 commits into from
Jan 2, 2019
Merged

Change sampler param sentinel value from YAML parser #145

merged 2 commits into from
Jan 2, 2019

Conversation

andremarianiello
Copy link
Contributor

@andremarianiello andremarianiello commented Dec 26, 2018 •
edited by yurishkuro
Loading

Which problem is this PR solving?

Short description of the changes

  • Instead of using 0 as the sentinel value for an unspecified sampler param, use another number that would be illegal instead. This allows param = 0 to be specified and respected by samplers.

@andremarianiello
Change sampler param sentinel value
ff9cdc2 
Signed-off-by: andremarianiello <andremarianiello@users.noreply.github.com>
@andremarianiello andremarianiello changed the title WIP: Change sampler param sentinel value Change sampler param sentinel value from YAML parser Dec 26, 2018
@yurishkuro
Copy link
Member

is it possible to add a unit test?

@andremarianiello
Add unit test for param=0
dfe64ed 
Signed-off-by: andremarianiello <andremarianiello@users.noreply.github.com>
@andremarianiello
Copy link
Contributor Author

@yurishkuro Added a test, which succeeds, and I verified that it failed without the sentinel value changes.

@isaachier
Copy link
Contributor

Let me take a look soon.

yurishkuro
yurishkuro approved these changes Dec 26, 2018
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@andremarianiello
Copy link
Contributor Author

Is there anything else I need to do for this?

@isaachier
Copy link
Contributor

Sorry thought @yurishkuro merged this. Will really try to get this in later today.

@isaachier isaachier merged commit fa8d376 into jaegertracing:master Jan 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andremarianiello @yurishkuro @isaachier