[GitHub] [hbase] timoha commented on pull request #1826: HBASE-24438 Don't update TaskMonitor when deserializing ServerCrashProcedure
timoha commented on pull request #1826: URL: https://github.com/apache/hbase/pull/1826#issuecomment-689901122 yup, the approach wasn't suitable enough to address the issue and I wasn't planning to improve it further, so I'll leave it to be properly addressed by hbase team This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] timoha commented on pull request #1826: HBASE-24438 Don't update TaskMonitor when deserializing ServerCrashProcedure
timoha commented on pull request #1826: URL: https://github.com/apache/hbase/pull/1826#issuecomment-654381936 Looks like operator intervention is needed for this issue :) > I for one do not look at TaskMonitor figuring state of Procedures. Do others? Thanks. Just from my perspective, I find it useful to see the procedure progress as looking plainly at procedure list isn't as helpful. In ideal world, I wouldn't need this information at all, as it would just do its job (and would only show something when it's broken). However, since this task exists, it should not have false positives. To make it clear, I'm against "improving" this side-effect as I wouldn't find it helpful to me as operator (I just don't care that something is de-serializing), that was just a suggestion that I now regret bringing up. I'm ok with closing this PR if you decide to go that way. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] timoha commented on pull request #1826: HBASE-24438 Don't update TaskMonitor when deserializing ServerCrashProcedure
timoha commented on pull request #1826: URL: https://github.com/apache/hbase/pull/1826#issuecomment-639009293 I'll let others also comment before submitting a patch. I personally prefer for de-serialize to not have side effects all the way to what shows in the UI and also like removing code rather than adding more :) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] timoha commented on pull request #1826: HBASE-24438 Don't update TaskMonitor when deserializing ServerCrashProcedure
timoha commented on pull request #1826: URL: https://github.com/apache/hbase/pull/1826#issuecomment-638533674 That's a good point @Apache9, I guess it would add sort of "continuity" to show that the task is still ongoing during master failover in case the procedure hasn't actually finished. However, I think there's an unwanted side effect here in case the procedure has actually completed by previous master (was marked done) and SCP not being actually scheduled for a while. As an operator, if I were to notice that there's SCP, I would freak out and try to see why my regionserver failed and then try to look at the logs (and in this case nothing is logged about it new master), and then try to look at procedure list (also without finding anything there). So, I guess in that case it would be more expected to show SCP in task when it's is actually being executed rather than "pending execution"? Maybe an alternative would be to improve the message in tasks and explicitly say "noticed an SCP, waiting for processing". Don't know if such verbosity would be useful though? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] timoha commented on pull request #1826: HBASE-24438 Don't update TaskMonitor when deserializing ServerCrashProcedure
timoha commented on pull request #1826: URL: https://github.com/apache/hbase/pull/1826#issuecomment-637745870 Thanks for suggestion @Apache9, but I don't think that's going to work for the case of state being `SERVER_CRASH_FINISH`. With such code change, `executeFromState` will end up calling `updateProgress(true)` at the beginning of the function marking the task as done. Whereas it should be actually marking the task as done when switch `case SERVER_CRASH_FINISH` is called. Also, could you please explain a rationale about adding a task during procedure deserialization? If procedure has finished during run of previous, what is the value in displaying it in tasks as "completed" when a new master comes after replaying the logs? In other words, should a simple fact of deserialization have such a side effect on tasks or should actual procedure execution drive the tasks updates instead? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
