bbeaudreault commented on pull request #4180: URL: https://github.com/apache/hbase/pull/4180#issuecomment-1079399239
Great feedback @apurtell. I hadn't thought of it like that, I mostly considered ServerOverloaded to be a far better name than literally "cqtbe" :). Now that I think about it.... tl;dr: - Make CQTBE extend CDE - Add new constructor to HBaseIOException which accepts `boolean serverOverloaded`, and default values where necessary to preserve compatibility. Note: I could add an intermediate exception if that would be preferable, like HBaseServerException or something. - Add ExceptionResponse.setServerOverloaded, and update server code to set this appropriately for the current cases. - Keep config `hbase.client.pause.server.overloaded`, but base it on `HBaseIOException.isServerOverloaded()` I agree with you that from an exception naming perspective, your feedback makes total sense. It's true that _today_ we might throw CDE/CQTBE in times of load, but that may not always be the case. I also agree with you that really CQTBE sounds more like a _reason_ for throwing a CallDroppedException. From a backoff perspective, I'm not sure I agree so much about wanting to attribute a special backoff for any possible future reason for dropping a call. You may not be suggesting that, but it's implied if we simply changed the exception name. I started thinking about it, in some ways RequestTooBigException is sort of a _reason_ to drop a call. But that extends DoNoRetryIOException. There may be other examples. So my takeaways so far are: - it probably makes sense to have a special pause for load issues specifically. So I personally like the `hbase.client.pause.server.overloaded` config. - that said, the exception (or the mode of sending "we're overloaded" state) might need some work. I was thinking about what a lightweight but good step forward here might be. What I came up with so far is we could add a new field to `ExceptionResponse`, i.e. `ExceptionResponse.setServerOverloaded(true)`. We could make CQTBE extend CDE (let me think more about compatibility tho), and we could add a new constructor to both which accepts the same `boolean serverOverloaded` value. We'd obviously keep a default constructor which sets that to false for compatibility. I don't think this would be much work -- I'd need to make some minor tweaks in the server code for the current handling of CQTBE and CDE. It'd also open the path to us decoupling server overload state from explicit exception class down the line, like if we wanted to pass that along in any other exceptions or as a separate signal. Thoughts? -- 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. To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
