Skip to content

opt: Fix flaky test#24715

Merged
craig[bot] merged 1 commit into
cockroachdb:masterfrom
andy-kimball:stress
Apr 12, 2018
Merged

opt: Fix flaky test#24715
craig[bot] merged 1 commit into
cockroachdb:masterfrom
andy-kimball:stress

Conversation

@andy-kimball
Copy link
Copy Markdown
Contributor

Update the TestNoMallocs to use the SucceedsSoon method in order
to repeatedly retry until zero memory allocations are detected.

Release note: None

@andy-kimball andy-kimball requested a review from a team as a code owner April 12, 2018 07:18
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@RaduBerinde
Copy link
Copy Markdown
Member

LGTM

@andy-kimball
Copy link
Copy Markdown
Contributor Author

bors r+


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@rytaft
Copy link
Copy Markdown
Collaborator

rytaft commented Apr 12, 2018

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


pkg/testutils/mem.go, line 34 at r1 (raw file):

		before := mem.Mallocs

		if err := fn(); err != nil {

When would this ever return an error?


Comments from Reviewable

Update the TestNoMallocs to use the SucceedsSoon method in order
to repeatedly retry until zero memory allocations are detected.

Release note: None
@andy-kimball
Copy link
Copy Markdown
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/testutils/mem.go, line 34 at r1 (raw file):

Previously, rytaft wrote…

When would this ever return an error?

I can't think of a good reason to return an error. I'll just remove that option.


Comments from Reviewable

@andy-kimball
Copy link
Copy Markdown
Contributor Author

bors r-

@craig
Copy link
Copy Markdown
Contributor

craig Bot commented Apr 12, 2018

Canceled

@rytaft
Copy link
Copy Markdown
Collaborator

rytaft commented Apr 12, 2018

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@andy-kimball
Copy link
Copy Markdown
Contributor Author

bors r+

craig Bot pushed a commit that referenced this pull request Apr 12, 2018
24529: distsql: collect tableReader stats and output to trace r=asubiotto a=asubiotto

This PR is split into two commits. The first commit lays the groundwork for stat collection code. The stats are modeled as a proto to be sent over the wire in the future (we will be sending these stats as metadata). The second commit modifies the tableReader to use this InputStatCollector to count the number of rows it reads. Example usage:

```
root@:26257/> SELECT COUNT(*) FROM test.xyz;
+-------+
| count |
+-------+
|     7 |
+-------+
(1 row)

Time: 257.249037ms

root@:26257/> SET tracing = ON;
SET

Time: 362.575µs

root@:26257/> SELECT x,y,z FROM test.xyz ORDER BY x;
+---+---+------+
| x | y |  z   |
+---+---+------+
| 1 | 1 | NULL |
| 1 | 1 |    2 |
| 1 | 1 |    2 |
| 1 | 2 |    1 |
| 2 | 2 |    3 |
| 4 | 5 |    6 |
| 4 | 1 |    6 |
+---+---+------+
(7 rows)

Time: 207.334037ms

root@:26257/> SHOW TRACE FOR SESSION;

... rest of trace elided ...

| 2018-04-05 20:53:01.571467+00:00 | 21s794ms540µs951ns | waiting for read lock                                                    | [n1,s1,r23/1:/Table/5{2-3}]                                |     | /cockroach.roachpb.Internal/Batch |   13 |
| 2018-04-05 20:53:01.777272+00:00 | 22s345µs429ns      | read completed                                                           | [n1,s1,r23/1:/Table/5{2-3}]                                |     | /cockroach.roachpb.Internal/Batch |   13 |
| 2018-04-05 20:53:01.777435+00:00 | 22s508µs797ns      | stat summary for tableReader input                                       | [client=[::1]:64047,user=root,n1,TableReader]              |     | table reader                      |   11 |
|                                  |                    |                                                                          |                                                            |     |                                   |      |
|                                  |                    |  rows read         7                                                     |                                                            |     |                                   |      |

... rest of trace elided ...
```
And then through jaeger:
<img width="1427" alt="screen shot 2018-04-05 at 4 53 55 pm" src="https://user-images.githubusercontent.com/10560359/38392141-740ae6d8-38f4-11e8-9406-2c167142eba6.png">

Note that we can't currently run `SHOW TRACE FOR <QUERY>` in distsql mode, which I hope to fix soon (#16562)

Any suggestions to improve the output welcome.

cc @knz @jordanlewis 

24715: opt: Fix flaky test r=andy-kimball a=andy-kimball

Update the TestNoMallocs to use the SucceedsSoon method in order
to repeatedly retry until zero memory allocations are detected.

Release note: None

24752: cli: Fix number of args expected by `debug gossip-values` r=a-robinson a=a-robinson

Fixes #24751

Release note: None

It looks like the problem is that my original usage string (`gossip-values <directory>`) was badly copied and pasted from other debug commands, and then assumed necessary by #24399 when in reality no positional argument is used by the command.

Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>
Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig Bot commented Apr 12, 2018

Build succeeded

@craig craig Bot merged commit c3b12f3 into cockroachdb:master Apr 12, 2018
@andy-kimball andy-kimball deleted the stress branch April 12, 2018 21:50
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.

4 participants