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

server, ui: Fix the range log history in the range debug page. #18376

Merged
merged 1 commit into from Sep 14, 2017

Conversation

BramGruneir
Copy link
Member

As reported in #18159, the range log query can take a long time to return. This solves this problem.

  • The rangeLog has been removed from the range debug endpoint so we can rely entirely on the preexisting rangelog endpoint. This required augmenting the endpoint to return pretty printed strings for all elements of the info as this cannot be done on the javascript side.
  • The range debug page now calls into this endpoint.
  • The orderBy clause has been removed from the query for now, since it was slowing down the select. This is currently being performed on the javascript side.
  • The limit is specificly removed from the request originating from the range debug page, as it also was impacting the performance of the query.
  • Since this new endpoint might still take a while to return, I gave the ajax call a rediculous amount of time before it times out and added the loading dialog to show that the request was still in progrss.
  • Moved the range log and simulated allocator to below connections, since those connections are different and the range log might now be quite large and will typically take longer to render.
  • Also, since some of the styles and fonts have changed, I updated the table's date column's width accordingly.

@BramGruneir BramGruneir added this to the 1.1 milestone Sep 8, 2017
@BramGruneir BramGruneir requested a review from a team as a code owner September 8, 2017 20:14
@BramGruneir BramGruneir requested a review from a team September 8, 2017 20:14
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@BramGruneir
Copy link
Member Author

There are only very small changes to looks in this change, so I'm not going to add a before/after. Just note that I've moved things around a bit on the range debug page, so that the elements that will take longer to populate, the range log and the allocator are now at the bottom.

@a-robinson
Copy link
Contributor

:lgtm_strong:


Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


pkg/server/admin.go, line 752 at r1 (raw file):

		q.Append(`WHERE "rangeID" = $ OR "otherRangeID" = $`, rangeID, rangeID)
	}
	// TODO(bram): Add this ordering back in after #18159 is resolved.

Is #18159 the issue you meant to point to? This PR will effectively resolve that issue, unless you plan to change what it's tracking.


pkg/server/admin.go, line 753 at r1 (raw file):

	}
	// TODO(bram): Add this ordering back in after #18159 is resolved.
	// q.Append("ORDER BY timestamp DESC ")

I think we only want to remove the ORDER BY if there's no limit -- otherwise we'll be returning a fairly arbitrary set of records.


pkg/server/serverpb/admin.proto, line 465 at r1 (raw file):

    PrettyInfo pretty_info = 2 [(gogoproto.nullable) = false];
  }
  reserved 1;

Nit, but personally I'd find it nice to also leave a comment indicating that this field has already been used in the past (as opposed to be reserved for some future use). Maybe that's just me, though.


pkg/server/serverpb/status.proto, line 499 at r1 (raw file):

    (gogoproto.nullable) = false
  ];
  reserved 4;

Ditto.


Comments from Reviewable

As reported in cockroachdb#18159, the range log query can take a long time to return. This solves this problem.

- The rangeLog has been removed from the range debug endpoint so we can rely entirely on the preexisting rangelog endpoint. This required augmenting the endpoint to return pretty printed strings for all elements of the info as this cannot be done on the javascript side.
- The range debug page now calls into this endpoint.
- The orderBy clause has been removed from the query for now, since it was slowing down the select.  This is currently being performed on the javascript side.
- The limit is specificly removed from the request originating from the range debug page, as it also was impacting the performance of the query.
- Since this new endpoint might still take a while to return, I gave the ajax call a rediculous amount of time before it times out and added the loading dialog to show that the request was still in progrss.
- Moved the range log and simulated allocator to below connections, since those connections are different and the range log might now be quite large and will typically take longer to render.
- Also, since some of the styles and fonts have changed, I updated the table's date column's width accordingly.
@BramGruneir
Copy link
Member Author

Review status: 9 of 17 files reviewed at latest revision, 4 unresolved discussions.


pkg/server/admin.go, line 752 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Is #18159 the issue you meant to point to? This PR will effectively resolve that issue, unless you plan to change what it's tracking.

I was thinking this was a temporary fix and we should re-add this once order by desc works correctly.

I've removed the comment, and if you ask for a limit, it now orders.


pkg/server/admin.go, line 753 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I think we only want to remove the ORDER BY if there's no limit -- otherwise we'll be returning a fairly arbitrary set of records.

hmm... good point. I've added it back in, but only when the results are limited.


pkg/server/serverpb/admin.proto, line 465 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Nit, but personally I'd find it nice to also leave a comment indicating that this field has already been used in the past (as opposed to be reserved for some future use). Maybe that's just me, though.

Done.


pkg/server/serverpb/status.proto, line 499 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Ditto.

Done.


Comments from Reviewable

@a-robinson
Copy link
Contributor

Reviewed 1 of 8 files at r2.
Review status: 10 of 17 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.


pkg/server/admin.go, line 752 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I was thinking this was a temporary fix and we should re-add this once order by desc works correctly.

I've removed the comment, and if you ask for a limit, it now orders.

ORDER BY DESC works correctly as of #18351, but it's still a lot slower than the unordered version (as tested on blue). So are we going to re-add it? If not now, when?

The improved speed of not doing the ordering is fantastic, so keeping out the ordering is nice. But I've also benefitted multiple times from being able to load the /_admin/v1/rangelog and see the events ordered, because having them ordered makes thrashing very obvious.


Comments from Reviewable

@BramGruneir
Copy link
Member Author

pkg/server/admin.go, line 752 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

ORDER BY DESC works correctly as of #18351, but it's still a lot slower than the unordered version (as tested on blue). So are we going to re-add it? If not now, when?

The improved speed of not doing the ordering is fantastic, so keeping out the ordering is nice. But I've also benefitted multiple times from being able to load the /_admin/v1/rangelog and see the events ordered, because having them ordered makes thrashing very obvious.

Yep. Going to leave this as is, and since the default still orders, I think we're good. If the amount of data coming back to the ui starts to get too large to display in the dom, we can start limiting it in the ui as well.


Comments from Reviewable

@BramGruneir BramGruneir merged commit 8f6cd3e into cockroachdb:master Sep 14, 2017
@BramGruneir BramGruneir deleted the 18159 branch September 14, 2017 19:47
@a-robinson
Copy link
Contributor

Make sure to cherry pick this, please :)

@BramGruneir
Copy link
Member Author

Of course!

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