Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
@zuyu @hbdeshmukh Ok -- looking at the code, I do think that having two
protos make sense. Down the line `RebuildWorkOrderCompletionMessage` may need
to report back compression ratio and/or block under-fill stats (for compressed
column stores). These won't make sense to include in the proto definition for
`NormalWorkOrderCompletionMessage`. So, let's keep these two protos separate.
We could do a union, but that is ugly and confusing code.
@zuyu has a good point that (if I read part of his concern correctly) that
we do need to record and report back `execution_time_in_microseconds` even for
`RebuildWorkOrderCompletionMessage`. So let's do that.
Also let's make a note in the comments around the proto definition about
why we have two protos (i.e. note that in the future
`RebuildWorkOrderCompletionMessage` will report back additional stats), so that
the reader is not confused.
I recommend @hbdeshmukh decide if he wants to fix this issue in this PR, or
open a separate PR relatively soon. If the latter is chosen, please make a note
here in the discussion thread for this PR.
In general, I'd recommend in situations like this to have a healthy
discussion. Then, if we are in a gray area, let us document the concern, and
give discretion to the person opening the PR.
I want to thank @zuyu for the diligent review -- without such careful
review, the code base could quickly become unmanageable. So keep it coming, and
don't hold back in the future.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
