fix(execute): properly use RefCount to reference count tables#1216
Merged
jsternberg merged 1 commit intomasterfrom May 14, 2019
Merged
fix(execute): properly use RefCount to reference count tables#1216jsternberg merged 1 commit intomasterfrom
jsternberg merged 1 commit intomasterfrom
Conversation
The consecutive transport and the result both use goroutines to process the tables and so need to have their own references to the tables. But, previously, the dataset was the only one that increased the reference count even though there was no guarantee it was using the consecutive transport and no guarantee that the reference count would be decremented. This modifies these two areas so that the reference count is increased on the call to `Process()` to signal that it will keep a reference to the table and then it decrements the reference count after it discards its usage of the table.
nathanielc
reviewed
Apr 29, 2019
| return msg.err | ||
| } | ||
| if err := f(msg.table); err != nil { | ||
| msg.table.RefCount(-1) |
Contributor
There was a problem hiding this comment.
Why does only this error branch decrement the counter?
Contributor
Author
There was a problem hiding this comment.
There's another line under this one for decrementing it in the normal situation. Let me refactor this so it uses a defer and anonymous function.
| } | ||
|
|
||
| func (t *consecutiveTransport) Process(id DatasetID, tbl flux.Table) error { | ||
| tbl.RefCount(1) |
Contributor
There was a problem hiding this comment.
Where is the companion decrement to the increment?
Contributor
Author
There was a problem hiding this comment.
It's in process message. It was already there previously, but the increment was in unrelated code so I thought it would be easier to track if the increment and decrement were in the same structure.
That's why I removed the increment in the dataset because it relied on an implementation detail.
nathanielc
approved these changes
May 14, 2019
jsternberg
added a commit
that referenced
this pull request
Jun 2, 2019
When the git log contained merges, the changelog generator didn't
necessarily work correctly. First, it would attempt to parse the message
in merge commits which was always wrong and always printed an error
message. Now, the changelog generator is told to just skip those commits
and move on since merge commits won't contain the message we're looking
for anyway.
The second happened with patch releases and feature branches that were
under work for more than a release. If a release was performed on the
maint branch and another commit was done on master before merging
maint back to master, the tree could be constructed in a way where the
previous version commit could not be clearly found such as in this
example from the `v0.31.0` release.
* 3204dd2 ci: add codecov.io support to flux (#1273)
* f0b6850 Merge branch 'maint'
|\
| * ec37117 (tag: v0.30.0) feat(universe): support for dynamic queries (#1284)
* | 2cce6fe feat(universe): support for dynamic queries (#1284)
|/
* 32fb2fb (tag: v0.29.0) fix(execute): properly use RefCount to reference count tables (#1216)
If the changelog generator encountered this pattern, it was undefined
exactly how it would behave. In this specific scenario, it parsed every
commit on the left branch going back to the root commit and then found
the previous commit on the second side of the branch. The opposite could
happen too and then it would have missed adding the commit from the
other branch.
The changelog generator has now been modified to find the merge base
before it attempts to read the log to prevent searching back through the
entire history. In the above situations, `v0.29.0` would be found and
act as a final constraint for parent commits.
The changelog parser has also been modified to use a breadth-first
search instead of depth-first search and it does not use the `Log()`
method from the go-git library. This is because the go-git library does
not have a way to stop at a specific commit and stop looking at parent
commits. We now limit the breadth-first search of commits to either the
merge base or the previous version commit which should be the same
commit if the release process was followed correctly. A breadth-first
search is used to ensure we get all commit messages from any merges and
don't skip any commits because we reached the terminating commit first.
jsternberg
added a commit
that referenced
this pull request
Jun 2, 2019
When the git log contained merges, the changelog generator didn't
necessarily work correctly. First, it would attempt to parse the message
in merge commits which was always wrong and always printed an error
message. Now, the changelog generator is told to just skip those commits
and move on since merge commits won't contain the message we're looking
for anyway.
The second happened with patch releases and feature branches that were
under work for more than a release. If a release was performed on the
maint branch and another commit was done on master before merging
maint back to master, the tree could be constructed in a way where the
previous version commit could not be clearly found such as in this
example from the `v0.31.0` release.
* 3204dd2 ci: add codecov.io support to flux (#1273)
* f0b6850 Merge branch 'maint'
|\
| * ec37117 (tag: v0.30.0) feat(universe): support for dynamic queries (#1284)
* | 2cce6fe feat(universe): support for dynamic queries (#1284)
|/
* 32fb2fb (tag: v0.29.0) fix(execute): properly use RefCount to reference count tables (#1216)
If the changelog generator encountered this pattern, it was undefined
exactly how it would behave. In this specific scenario, it parsed every
commit on the left branch going back to the root commit and then found
the previous commit on the second side of the branch. The opposite could
happen too and then it would have missed adding the commit from the
other branch.
The changelog generator has now been modified to find the merge base
before it attempts to read the log to prevent searching back through the
entire history. In the above situations, `v0.29.0` would be found and
act as a final constraint for parent commits.
The changelog parser has also been modified to use a breadth-first
search instead of depth-first search and it does not use the `Log()`
method from the go-git library. This is because the go-git library does
not have a way to stop at a specific commit and stop looking at parent
commits. We now limit the breadth-first search of commits to either the
merge base or the previous version commit which should be the same
commit if the release process was followed correctly. A breadth-first
search is used to ensure we get all commit messages from any merges and
don't skip any commits because we reached the terminating commit first.
jsternberg
added a commit
that referenced
this pull request
Jun 3, 2019
When the git log contained merges, the changelog generator didn't
necessarily work correctly. First, it would attempt to parse the message
in merge commits which was always wrong and always printed an error
message. Now, the changelog generator is told to just skip those commits
and move on since merge commits won't contain the message we're looking
for anyway.
The second happened with patch releases and feature branches that were
under work for more than a release. If a release was performed on the
maint branch and another commit was done on master before merging
maint back to master, the tree could be constructed in a way where the
previous version commit could not be clearly found such as in this
example from the `v0.31.0` release.
* 3204dd2 ci: add codecov.io support to flux (#1273)
* f0b6850 Merge branch 'maint'
|\
| * ec37117 (tag: v0.30.0) feat(universe): support for dynamic queries (#1284)
* | 2cce6fe feat(universe): support for dynamic queries (#1284)
|/
* 32fb2fb (tag: v0.29.0) fix(execute): properly use RefCount to reference count tables (#1216)
If the changelog generator encountered this pattern, it was undefined
exactly how it would behave. In this specific scenario, it parsed every
commit on the left branch going back to the root commit and then found
the previous commit on the second side of the branch. The opposite could
happen too and then it would have missed adding the commit from the
other branch.
The changelog generator has now been modified to find the merge base
before it attempts to read the log to prevent searching back through the
entire history. In the above situations, `v0.29.0` would be found and
act as a final constraint for parent commits.
The changelog parser has also been modified to use a breadth-first
search instead of depth-first search and it does not use the `Log()`
method from the go-git library. This is because the go-git library does
not have a way to stop at a specific commit and stop looking at parent
commits. We now limit the breadth-first search of commits to either the
merge base or the previous version commit which should be the same
commit if the release process was followed correctly. A breadth-first
search is used to ensure we get all commit messages from any merges and
don't skip any commits because we reached the terminating commit first.
jsternberg
added a commit
that referenced
this pull request
Jun 3, 2019
…1319) When the git log contained merges, the changelog generator didn't necessarily work correctly. First, it would attempt to parse the message in merge commits which was always wrong and always printed an error message. Now, the changelog generator is told to just skip those commits and move on since merge commits won't contain the message we're looking for anyway. The second happened with patch releases and feature branches that were under work for more than a release. If a release was performed on the maint branch and another commit was done on master before merging maint back to master, the tree could be constructed in a way where the previous version commit could not be clearly found such as in this example from the `v0.31.0` release. * 3204dd2 ci: add codecov.io support to flux (#1273) * f0b6850 Merge branch 'maint' |\ | * ec37117 (tag: v0.30.0) feat(universe): support for dynamic queries (#1284) * | 2cce6fe feat(universe): support for dynamic queries (#1284) |/ * 32fb2fb (tag: v0.29.0) fix(execute): properly use RefCount to reference count tables (#1216) If the changelog generator encountered this pattern, it was undefined exactly how it would behave. In this specific scenario, it parsed every commit on the left branch going back to the root commit and then found the previous commit on the second side of the branch. The opposite could happen too and then it would have missed adding the commit from the other branch. The changelog generator has now been modified to find the merge base before it attempts to read the log to prevent searching back through the entire history. In the above situations, `v0.29.0` would be found and act as a final constraint for parent commits. The changelog parser has also been modified to use a breadth-first search instead of depth-first search and it does not use the `Log()` method from the go-git library. This is because the go-git library does not have a way to stop at a specific commit and stop looking at parent commits. We now limit the breadth-first search of commits to either the merge base or the previous version commit which should be the same commit if the release process was followed correctly. A breadth-first search is used to ensure we get all commit messages from any merges and don't skip any commits because we reached the terminating commit first.
davidgs
pushed a commit
to davidgs/flux
that referenced
this pull request
Aug 20, 2019
…data#1216) The consecutive transport and the result both use goroutines to process the tables and so need to have their own references to the tables. But, previously, the dataset was the only one that increased the reference count even though there was no guarantee it was using the consecutive transport and no guarantee that the reference count would be decremented. This modifies these two areas so that the reference count is increased on the call to `Process()` to signal that it will keep a reference to the table and then it decrements the reference count after it discards its usage of the table.
davidgs
pushed a commit
to davidgs/flux
that referenced
this pull request
Aug 20, 2019
…nfluxdata#1319) When the git log contained merges, the changelog generator didn't necessarily work correctly. First, it would attempt to parse the message in merge commits which was always wrong and always printed an error message. Now, the changelog generator is told to just skip those commits and move on since merge commits won't contain the message we're looking for anyway. The second happened with patch releases and feature branches that were under work for more than a release. If a release was performed on the maint branch and another commit was done on master before merging maint back to master, the tree could be constructed in a way where the previous version commit could not be clearly found such as in this example from the `v0.31.0` release. * 3204dd2 ci: add codecov.io support to flux (influxdata#1273) * f0b6850 Merge branch 'maint' |\ | * ec37117 (tag: v0.30.0) feat(universe): support for dynamic queries (influxdata#1284) * | 2cce6fe feat(universe): support for dynamic queries (influxdata#1284) |/ * 32fb2fb (tag: v0.29.0) fix(execute): properly use RefCount to reference count tables (influxdata#1216) If the changelog generator encountered this pattern, it was undefined exactly how it would behave. In this specific scenario, it parsed every commit on the left branch going back to the root commit and then found the previous commit on the second side of the branch. The opposite could happen too and then it would have missed adding the commit from the other branch. The changelog generator has now been modified to find the merge base before it attempts to read the log to prevent searching back through the entire history. In the above situations, `v0.29.0` would be found and act as a final constraint for parent commits. The changelog parser has also been modified to use a breadth-first search instead of depth-first search and it does not use the `Log()` method from the go-git library. This is because the go-git library does not have a way to stop at a specific commit and stop looking at parent commits. We now limit the breadth-first search of commits to either the merge base or the previous version commit which should be the same commit if the release process was followed correctly. A breadth-first search is used to ensure we get all commit messages from any merges and don't skip any commits because we reached the terminating commit first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The consecutive transport and the result both use goroutines to process
the tables and so need to have their own references to the tables. But,
previously, the dataset was the only one that increased the reference
count even though there was no guarantee it was using the consecutive
transport and no guarantee that the reference count would be
decremented.
This modifies these two areas so that the reference count is increased
on the call to
Process()to signal that it will keep a reference tothe table and then it decrements the reference count after it discards
its usage of the table.