gortiz commented on PR #16919: URL: https://github.com/apache/pinot/pull/16919#issuecomment-3360210507
>Per my understanding, this FIRST_ERROR status was introduced because it expects sender to call onCompleted() after sending the error block. This is probably unnecessary, and we can directly close the stream on the receiver side when getting error block from sender. I introduced FIRST_ERROR in https://github.com/apache/pinot/pull/12704 to replicate the older behavior and I even added a comment in `https://github.com/apache/pinot/pull/12704/files#r1565395943`. A correctly implemented sender should call onComplete after sending the EOS during the happy path, but that is not guarantee. The thread sending thread could for example be interrupted. We cannot trust that the other side will call onComplete. Closing our side is safer and correct. > Do you see value differentiating FIRST_ERROR and ERROR? It it very confusing keeping both of them Yes. FIRST_ERROR means: The offered message is an error and no more messages are expected on the receiving mailbox. This is something expected and shouldn't need to be logged. On the other hand, ERROR means: The receiving mailbox was already closed by a previous error before the last block was offered. This is unexpected and probably a bug, so it should be logged. Anyway I'm changing all these enums in https://github.com/apache/pinot/pull/16903. I hope the code in that PR is going to be easier to understand, as it is heavily documented. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
