vvcephei commented on pull request #11567: URL: https://github.com/apache/kafka/pull/11567#issuecomment-1002770012
Hey @patrickstuedi , thanks for the last batch of updates. Aside from the build failure, I think these are my last comments: 1. After scrolling around the PR a few times to try and clarify whether we're handling all the right cases, I'm more convinced now that we really should consolidate all the query handling logic into StoreQueryUtils. I gather from your comment that you feel the same way. Let's just go for it in this PR. The thing you saw in the other PR was just an oversight. 2. Now that I'm really confronted with the reality of the code, I think it's going to be pretty confusing for people that WindowRangeQuery only works with WindowStore when you do withWindowStartRange(from, to) and it only works on SessionStore when you do withKey(key). I still think it's ok to do that, but I think we ought to add more description to the failure message than just "this store doesn't know how to handle this query". Can you replace the else cases there with QueryResult.forFailure(FailureReason.UNKNOWN_QUERY_TYPE, "...") and a message that more accurately explains what the problem is? It would also be nice to have negative test cases for these to verify the correct messages makes it to the user. Once those points are addressed, we should be ready to merge! -- 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]
