Skip to content
This repository was archived by the owner on Jan 28, 2021. It is now read-only.

Add progress for each partition in SHOW PROCESSLIST #855

Merged
merged 2 commits into from
Oct 24, 2019

Conversation

carlosms
Copy link
Contributor

This is a proposal to add some more information to the SHOW PROCESSLIST output.
It adds a counter for each active partition. A partition gets removed from the list when it finishes.

This is how it looks:

$ mysql -u root -h 127.0.0.1 -P 3306 gitbase --execute "show processlist"
+------+------+-----------------+---------+---------+------+------------------------------------------------------------------+-----------------------+
| Id   | User | Host            | db      | Command | Time | State                                                            | Info                  |
+------+------+-----------------+---------+---------+------+------------------------------------------------------------------+-----------------------+
|  395 | root | 127.0.0.1:35250 | gitbase | query   |    2 | cangallo(45/?), commits(0/3), octoprint-tft(45/?), upsilon(45/?) | select * from commits |
|  396 | root | 127.0.0.1:35250 | gitbase | query   |    0 | running                                                          | show processlist      |
+------+------+-----------------+---------+---------+------+------------------------------------------------------------------+-----------------------+

$ mysql -u root -h 127.0.0.1 -P 3306 gitbase --execute "show processlist"
+------+------+-----------------+---------+---------+------+------------------------------+-----------------------+
| Id   | User | Host            | db      | Command | Time | State                        | Info                  |
+------+------+-----------------+---------+---------+------+------------------------------+-----------------------+
|  395 | root | 127.0.0.1:35256 | gitbase | query   |   11 | commits(2/3), upsilon(229/?) | select * from commits |
|  398 | root | 127.0.0.1:35256 | gitbase | query   |    0 | running                      | show processlist      |
+------+------+-----------------+---------+---------+------+------------------------------+-----------------------+
$ mysql -u root -h 127.0.0.1 -P 3306 gitbase --execute "show processlist"
+------+------+-----------------+---------+---------+------+------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------+
| Id   | User | Host            | db      | Command | Time | State                                                                                                      | Info                                                               |
+------+------+-----------------+---------+---------+------+------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------+
|  173 | root | 127.0.0.1:35236 | gitbase | query   |   65 | SquashedTable(commits, commit_blobs, blobs)(0/3), cangallo(1289/?), octoprint-tft(1280/?), upsilon(1281/?) | select * from commits natural join commit_blobs natural join blobs |
|  393 | root | 127.0.0.1:35236 | gitbase | query   |    0 | running                                                                                                    | show processlist                                                   |
+------+------+-----------------+---------+---------+------+------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------+
$ mysql -u root -h 127.0.0.1 -P 3306 gitbase --execute "show processlist"
+------+------+-----------------+---------+--------------+------+------------------------------+---------------------------------------------------------------------------------------------------------+
| Id   | User | Host            | db      | Command      | Time | State                        | Info                                                                                                    |
+------+------+-----------------+---------+--------------+------+------------------------------+---------------------------------------------------------------------------------------------------------+
|  395 | root | 127.0.0.1:35270 | gitbase | create_index |    8 | commits(2/3), upsilon(160/?) | CREATE INDEX commits_hash_idx ON commits USING pilosa (repository_id, commit_hash) WITH (async = false) |
|  402 | root | 127.0.0.1:35270 | gitbase | query        |    0 | running                      | show processlist                                                                                        |
+------+------+-----------------+---------+--------------+------+------------------------------+---------------------------------------------------------------------------------------------------------+

The new output can be used to notice when for some reason a repository takes more time than others to finish.

Because there aren't totals, the counter of each partition cannot be used to get an estimate of the progress made. Right now its only function is to let the user know that the iterator is working.

I am concerned with the onNext function, because I'm not sure if updating the progress for every row will have an impact on performance. I'm working on testing this, but so far gitbase-regression has been giving me unreliable times.

If we removed onNext we could still benefit from having the name of the active partitions, maybe with a (?/?) progress.

Another generic improvement would be to have a way to get the total number of rows for a table. Maybe implementing a new interface. I didn't explore this possibility because as far as I know in gitbase we won't be able to use this. But it may be an interesting generic addition later for go-mysql-server itself.

@carlosms carlosms requested a review from a team October 23, 2019 16:18
@erizocosmico
Copy link
Contributor

Overall the functionality and the implementation look good to me, but I just have one doubt: what is the expected way of using this in a client (i.e. gitbase)?

I see ProcessTable having the callbacks, but the client is not supposed to touch ProcessTable at all. In fact, it's a node for internal use and the user should never have to even know it exists.

Copy link
Contributor

@juanjux juanjux left a comment

Choose a reason for hiding this comment

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

LGTM after considering what @erizocosmico said.

@carlosms
Copy link
Contributor Author

what is the expected way of using this in a client (i.e. gitbase)?

gitbase doesn't need to change at all. All the progress is tracked with the changes in this PR, the callbacks are called by the trackedRowIter wrapper.

There is one caveat I didn't mention. The name of the Partition is made from the Partition Key() []byte. In the case of gitbase the key is the repository name, so it's a good user-facing ID.
But thinking as a generic lib, this Key might be an ugly or random []byte.

For gitbase the implementation works as it is. But we might want to add Name to the Partition interface to make sure the clients return a nice looking name for each partition:

type Partition interface {
	Nameable
	Key() []byte
}

or leave the Partition interface as it is, and clients can optionally implement Nameable for their partitions. Then do type assertion on sql.Nameable and use the Name() if it's possible, or the Key() as a fallback.

The first option is a breaking change for a feature that might not be too relevant for other users. The second one is not breaking, but will be completely missed and it's probably easier to just leave the code as it is now, and assume the Key() []byte returns a user-readable string.

WDYT?

@erizocosmico
Copy link
Contributor

I think 2nd option might be best

@carlosms
Copy link
Contributor Author

Done in a385e34.

@erizocosmico erizocosmico merged commit 68b087c into src-d:master Oct 24, 2019
@carlosms carlosms deleted the progress-info branch October 24, 2019 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants