hack for GRPC execution plans #1684
Conversation
| // scalastyle:off number.of.types | ||
| // scalastyle:off file.size.limit | ||
| // scalastyle:off method.length | ||
| object ProtoConverters { |
There was a problem hiding this comment.
Why not use the existing ProtoConverters, especially when the ExecPlans are in query? Please use the same object and add your converters there. Also make sure you tests are written for each and every converter.
There was a problem hiding this comment.
dispatchers are in the coordinator, otherwise, I would have kept them in the original ProtoConverters, in fact I started writing the logic in the original ProtoConverters.
| @@ -44,7 +42,15 @@ case class ActorPlanDispatcher(target: ActorRef, clusterName: String) extends Pl | |||
| emptyPartialResult | |||
| }) | |||
| } else { | |||
| val fut = (target ? plan.execPlan) (t).map { | |||
| // HACK | |||
There was a problem hiding this comment.
Is this really a hack?
There was a problem hiding this comment.
yes, this is a hack, not meant to be committed, this PR is just a POC that shows how proto exec plans can works end to end and solicit the feedback. Here, for example the question is how would we want to proceed with the configuration switch that would enable proto serialization for the exec plans. I think the most preferable option is to pass it as a query parameter, smth like protoExec=true.
| @@ -3,13 +3,10 @@ package filodb.query | |||
|
|
|||
| import java.util.concurrent.TimeoutException | |||
| import java.util.concurrent.atomic.{AtomicInteger, AtomicLong} | |||
|
|
|||
| import scala.collection.JavaConverters._ | |||
There was a problem hiding this comment.
These empty lines are needed for style checks, add them back.
There was a problem hiding this comment.
sorry, this is just a typo
|
Please follow these guidelines to update the PR title and give some description for the PR. Also I will avoid the word hack :) as it isn't one. |
Pull Request checklist
Current behavior : (link exiting issues here : https://help.github.com/articles/basic-writing-and-formatting-syntax/#referencing-issues-and-pull-requests)
New behavior :
BREAKING CHANGES
If this PR contains a breaking change, please describe the impact and migration
path for existing applications.
If not please remove this section.
Breaking changes may include:
Other information: