Uh oh!
There was an error while loading. Please reload this page.
HBASE-21521 Expose master startup status via web UI - #3667
Open
shahrs87 wants to merge 1 commit into
Open
Conversation
Apache-HBase
commented
Sep 8, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2021
🎊 +1 overall
This message was automatically generated. |
ContributorAuthor
@apurtell@saintstack Could you please help review this PR ? Screenshots are attached in jira. Thank you ! |
joshelser
requested changes
Sep 10, 2021
joshelser
left a comment
Member
There was a problem hiding this comment.
We have a MonitoredTask for tracking HMaster startup which is exposed in the "Tasks" header on the Master UI today.
What is your solution here doing differently than what we already have with MonitoredTask and the Tasks page on the Master UI?
If there's a gap in understanding/visibility, I think it'd be better to reuse what we already have than create something brand new. With this change, we would have two things which are "tracking" master startup.
| */ | ||
| @InterfaceAudience.Private | ||
| public class PhaseStatus { | ||
| private String phaseName; |
| * a clone of the data. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class MasterStartupProgress { |
Member
There was a problem hiding this comment.
We have MonitoredTask and MonitoredTaskImpl which appear to do very similar things to what you're doing here.
Did you look at MonitoredTask and is there a reason we cannot use/extend it instead of writing net-new code?
Apache-HBase
commented
Sep 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 10, 2021
💔 -1 overall
This message was automatically generated. |
shahrs87
commented
Sep 10, 2021
ContributorAuthor
Thank you @joshelser for the feedback.
The current Tasks page is not very clear in the sense it doesn't give me all the different phases view. Once the startup finishes, all the tasks disappears. Also there are some limitations in MonitoredTask which doesn't allow me to track start time and end time for each state. Also it doesn't show the states that are completed. Since MonitoredTask is being used at many places, I didn't want to change just for HMaster startup use case. WDYT ? |
apurtell
requested changes
Sep 14, 2021
Contributor
There was a problem hiding this comment.
It works, but it would be cleaner if somehow Phases can be MonitoredTasks. Take what we have already for task monitoring and add the ability to group them. Startup activity can be the first official grouping.
TaskGroup createTaskGroup(String name)
TaskGroup getTaskGroup(String name)
TaskGroup#addTask(MonitoredTask task)
Collection<MonitoredTask> TaskGroup#getTasks()
etc.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.hadoop.hbase.master.startupprogress; |
Contributor
There was a problem hiding this comment.
This all seems redundant with what MonitoredTask provides.
| */ | ||
| @InterfaceAudience.Private | ||
| public class PhaseTracking { | ||
| private long startTime; |
Contributor
There was a problem hiding this comment.
This is all redundant with what MonitoredTask provides.
Apache-HBase
commented
Sep 15, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 15, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 15, 2021
💔 -1 overall
This message was automatically generated. |
joshelser
commented
Sep 15, 2021
Member
IMO, if start/end time is the only thing you're missing, then just add that to MonitoredTask and adapt your UI to use that class instead. I think that Andrew came to the same conclusion about duplicating MonitoredTask, the best idea is to try to reuse it :) The UI component is great and I agree that the "Tasks" page currently on the UI leaves something to be desired. I think your presentation of this data is excellent. |
Apache-HBase
commented
Sep 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 23, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Sep 23, 2021
ContributorAuthor
Test passing on my local cluster: |
Apache-HBase
commented
Sep 23, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Sep 24, 2021
ContributorAuthor
Attached 2 new screenshots in HBASE-21521. Attaching here also. This now uses the @joshelser@apurtell please review again. Thank you ! |
joshelser
requested changes
Sep 30, 2021
joshelser
left a comment
Member
There was a problem hiding this comment.
I like this approach a lot better! Good work.
One concern I have which this new approach inadvertently creates is that the Master will potentially OOME itself if it's waiting for RegionServers to come up for a long period of time. The journal in MonitoredTaskImpl can grow unbounded. Thus, like you pointed out where we get multiple journal entries for the same "waiting for regionservers" update, we continue to hold those in memory for (best as I can tell) the lifetime of the HBase Master.
Every WAIT_ON_REGIONSERVERS_TIMEOUT seconds (which defaults to 4.5 seconds), it will create a new status with updated message. I like the way it tells the operator that how many RS it was waiting for to become online.
What do you think about one more extension to MonitoredTask/StatusJournal such that if we publish the same StatusJournalEntry multiple times, we drop the tail of that journal before we add the new entry to the journal. From the POV of the user, they would see the latest "waiting on regionservers" log message with the time incrementing, but this UI page wouldn't grow unbounded.
| List<Phase> phases = new ArrayList<>(); | ||
| // Make a copy of the list to avoid ConcurrentModificationException. | ||
| List<MonitoredTask.StatusJournalEntry> journalEntries = | ||
| ImmutableList.copyOf(this.monitoredTask.getStatusJournal()); |
Member
There was a problem hiding this comment.
I wouldn't expect the caller to be modifying this list (especially given that Phase should also be immutable).
How about avoid the copy by using Collections.unmodifiableList()?
| if (elapsedTime == Long.MAX_VALUE) { | ||
| return "-"; | ||
| } | ||
| long hours, minutes, seconds, milliseconds; |
Member
There was a problem hiding this comment.
HBase follows https://www.oracle.com/java/technologies/javase/codeconventions-declarations.html which avoids multiple initializations on the same line. I believe we do not care if they are done in a block at the start of the method (each on their own line) or if you declare them as you use them the first time.
shahrs87
commented
Oct 11, 2021
ContributorAuthor
@joshelser It's tricky to find out which StatusJournalEntry are the same. How about we keep the last 500 status in memory and evict the older ones ? |
joshelser
commented
Oct 11, 2021
Member
That's acceptable to start. I think this is not ideal as, if the system is stuck in this situation where we are waiting on regionservers (or similar), we will eventually lose all of the "good" status updates prior to this. That said, we should also make sure "perfect" isn't the enemy of "good" :) |
Apache9
commented
Nov 21, 2021
Contributor
Any progress here? |
This was referenced Sep 13, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.