Skip to content

fix(execute): properly use RefCount to reference count tables#1216

Merged
jsternberg merged 1 commit intomasterfrom
fix/table-refcounts
May 14, 2019
Merged

fix(execute): properly use RefCount to reference count tables#1216
jsternberg merged 1 commit intomasterfrom
fix/table-refcounts

Conversation

@jsternberg
Copy link
Copy Markdown
Contributor

@jsternberg jsternberg commented Apr 25, 2019

  • docs/SPEC.md updated
  • Test cases written

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.

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.
return msg.err
}
if err := f(msg.table); err != nil {
msg.table.RefCount(-1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why does only this error branch decrement the counter?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is the companion decrement to the increment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@jsternberg jsternberg merged commit 32fb2fb into master May 14, 2019
@jsternberg jsternberg deleted the fix/table-refcounts branch May 14, 2019 19:49
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.
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