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

Add an Update method to update an existing tag #24

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

vincentbernat
Copy link
Contributor

This is useful to avoid a double lookup: once to find the current
value, the second time to update its value. The tag could be a counter
we want to update or, with the generics tree, any struct/map.

The code change is minimal but Set() could be a bit smaller since it
reuses the same code path. However, I believe the compiler should be
smart enough to turn the constant function into a single value.

@coveralls
Copy link

coveralls commented Aug 22, 2022

Pull Request Test Coverage Report for Build 2905847624

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 81.215%

Totals Coverage Status
Change from base Build 2834590035: 0.0%
Covered Lines: 147
Relevant Lines: 181

💛 - Coveralls

@vincentbernat
Copy link
Contributor Author

The detected lint issue is incorrect. The _ is quite important as the tag may not exist. If you think the PR is OK otherwise, I'll use a workaround.

@vincentbernat
Copy link
Contributor Author

My bad, I always thought Go would panic when looking up a non-existing key. That's not true!

@vincentbernat vincentbernat marked this pull request as draft August 25, 2022 07:52
@vincentbernat
Copy link
Contributor Author

I'll update the PR shortly with an updated and more generic approach.

if t.nodes[nodeIndex].TagCount == 0 {
t.nodes[nodeIndex].TagCount = 1
} else {
ret = false
}
t.tags[(uint64(nodeIndex) << 32)] = tag
t.tags[(uint64(nodeIndex) << 32)] = updateFunc(t.tags[(uint64(nodeIndex) << 32)])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is incorrect as the existing tag could be random when TagCount was 0. That's one reason I need to work a bit more on this PR.

This is useful to avoid a double lookup: once to find the current
value, the second time to update its value. The tag could be a counter
we want to update or, with the generics tree, any struct/map.

`SetOrUpdate()` is the same as `Set()`, but when there already is a
tag, it is passed to the provided update function instead of being
replaced by the provided tag value. If no match, the provided tag
value is used instead.

`AddOrUpdate()` is the same as `Add()` and works like `Set()` except
it will append a tag (when no match) or modify the first matching
tag (when there is a match).

Both functions take a tag value which is used when the existing tag
does not exist (no tag for `SetOrUpdate()` or no matching tag for
`AddOrUpdate()`). Both functions take an update function which is used
when there is an existing tag and the update function is applied on
the existing tag to get another tag. The provided tag is therefore
ignored in this case.
@vincentbernat vincentbernat marked this pull request as ready for review August 25, 2022 08:54
@vincentbernat
Copy link
Contributor Author

Here is the second tentative. The differences:

  • There is a now an AddOrUpdate() function and Update() is renamed to SetOrUpdate().
  • The update function is only used when there is an existing tag (which would have been replaced with the non-update version). When there is no match, the provided tag is used instead. It is more intuitive than calling the update function on the zero value.

The addTag() function is also simpler and handle both Set*() and Add*() methods the same way.

@aaronfuj aaronfuj merged commit 96387d0 into kentik:main Oct 21, 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.

None yet

3 participants