-
Notifications
You must be signed in to change notification settings - Fork 17
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
Show partition replica usage and limit in kafka:info #168
Conversation
test/commands/info_test.js
Outdated
Version: 0.10.0.0 | ||
Created: 2016-11-14T14:26:20.245+00:00 | ||
Topics: 1 / 10 topics, see heroku kafka:topics | ||
Partitions: 12 / 32 partition replicas (partitions * replication factor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little awkward, but throwing it out there as a straw man. Suggestions welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uhoh-itsmaciek I think we want to show one or the other, but not both. If limits.max_partition_replica_count
exists, that is the metric we want to show since topics can vary in partition replica size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justindowning One or the other what? This is showing replica counts. The label is the shorter Partitions
only because the full "Partition Replicas" is too long and putting it there misaligns the whole table and makes it look awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uhoh-itsmaciek does our docs talk about partition replicas? i wonder if we can find some way to simplify this.
In many cases, the replication factor is fixed, so we could show partition count instead of partition replica count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know re: docs. But that seems like it could be really confusing for cases where someone has actually changed the RF. What were you thinking of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@idan yeah, Dev Center needs updating. MT Kafka will drive a handful of updates to the docs, due to the way the plans are structured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rand @heroku/dod-kcz the other thing we could do is limit based only on partitions (not partition replicas), and fudge the numbers so that with a common replication factor, you're expected to hit the limit of partition replicas close to what we want. This could work because even when the replication factor is not fixed, it's pretty tightly bounded (i.e., min 3, max 8).
E.g., instead of limiting you to 500 partition replicas, we limit you to 100 partition (with the assumption that your average RF is ~5, so you'll end up using ~500 partition replicas).
The downside is that we need to come up with a heuristic typical RF. If we guess too high, we'll constrain users more than necessary (and they'll run out of partitions as per our limit while the cluster could technically handle more). If we guess too low, users may end up running out of resources on the cluster because they're using more partition replicas than we expected (while still within the partition limit).
That is, this would simplify the user-facing limits, but at the cost of forcing us into heuristic calculations to actually limit the resource usage.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be concerned about any heuristic we could come up with for the RF. While it's effectively fixed at 3 on standard
plans, and actually fixed at 3 on mt-*
plans, the extended
case can get really weird... and those are going to be our power users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we've used my smooth-progress in a couple of places: https://github.com/dickeyxxx/smooth-progress
not really applicable for this, but may be useful if you want to see how to use unicode. I think using the =
sign in this display is fine though. It feels like an element we could make use of in other places, but nothing is coming to mind right now. There is also sparkline that I use for heroku dashboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rand yup, okay, makes sense. Just throwing that out there.
67faf24
to
67e56b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
test/commands/info_test.js
Outdated
Version: 0.10.0.0 | ||
Created: 2016-11-14T14:26:20.245+00:00 | ||
Topics: [█ ] 1 / 10 topics, see heroku kafka:topics | ||
Partitions: [███▊ ] 12 / 32 partition replicas (partitions × replication factor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 assuming the bar is just an issue with my environment... |
Some fonts are weird with these, I don't think there is a real fix |
Hmm... I'd like to understand the impact before we go with that, then. I don't think the bars add enough to risk artifacts like this for an unknown number of users... |
Bah. I'm going to roll back the pixelbar thing and just go with full-blocks only. I dug into this a little and it turns out that it depends on the vagaries of both which typefaces you have installed and your OS's resolution strategy when your preferred typefaces lack the displayed glyphs. The height mismatches come from these missing glyphs leading to fallbacks to typefaces with different metrics. |
Ok, nice. And you're truncating, right? If I have 2999 / 3000 partition replicas, that still leaves one bar un-filled? |
This drops pixelBar in favor of a builtin utilizationBar that does not use awesome unicode to fill partial blocks, because this leads to font issues.
@uhoh-itsmaciek no, I'm rounding — if you have 2999/3000, you are way closer to 100% than you are to 90%. The bar is a quick visual guide and will never tell a precise story, which is why we aren't getting rid of the numbers next to it. Do you feel strongly about this? I kinda stuck my finger in the breeze, but that's my rationale. |
I feel like it makes sense to use floor vs round. But I may be colored by this one example |
Me too, but not strongly. You can ship as is. |
@dickeyxxx @uhoh-itsmaciek ok, using floor now, with updated tests. Ready for review! |
+1 |
No description provided.