You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by Navneet Potti <na...@gmail.com> on 2016/06/15 04:36:41 UTC
Do we need 3 different InsertDestination classes?
Hi Harshad
I’m just kicking off this discussion based on a conversation I had with Jignesh this morning. We have 3 different implementations of InsertDestination, and I think we only really need one, the PartitionAwareBlockPoolInsertDestination.
The AlwaysCreateBlockInsertDestination seems to be entirely useless practically. I’m guessing it’s a relic of early development. The normal BlockPoolInsertDestination seems like a degenerate case of the PartitionAware one, with a single partition.
If my understanding of the design is right, then we should refactor this code to only have one InsertDestination. There is a lot of duplication of code and complex logic in there. Anything we can do to simplify and clean up the code is probably a good move.
Thoughts?
Cheers,
Navneet
Re: Do we need 3 different InsertDestination classes?
Posted by Adalbert Gerald Soosai Raj <so...@wisc.edu>.
We should remove the AlwaysCreateBlockInsertDestination since it is not
useful at all.
For the reasons that Harshad has pointed out, we should keep the
BlockPoolInsertDestination.
On Wed, Jun 15, 2016 at 9:31 AM, Harshad Deshmukh <ha...@cs.wisc.edu>
wrote:
> Hi Navneet,
>
> I was always in favor of getting rid of AlwaysCreateBlock derived class.
> This issue was raised long time back, but Craig insisted that we keep it.
>
> I guess the PartitionAware class can't be an efficient drop in replacement
> for the BlockPool class. The reason is that it checks for each input tuple,
> which output partition should it be sent to. This is an unnecessary step
> when we don't have partitioning enabled.
>
> An alternative is something called PartitionPreserving derived class, that
> Gerald wrote. It skips the above check that I mentioned.
>
> Gerald, please feel free to correct if I didn't get it right.
>
>
> On 06/15/2016 03:20 AM, Jignesh Patel wrote:
>
>> A huge +1 from me to get rid of dead code!
>>
>> +CC: Gerald who also worked on this a while back, in case he has some
>> input.
>>
>> Thanks!
>> Jignesh
>>
>>
>> On Jun 14, 2016, at 11:36 PM, Navneet Potti <na...@gmail.com> wrote:
>>>
>>> Hi Harshad
>>> I’m just kicking off this discussion based on a conversation I had with
>>> Jignesh this morning. We have 3 different implementations of
>>> InsertDestination, and I think we only really need one, the
>>> PartitionAwareBlockPoolInsertDestination.
>>> The AlwaysCreateBlockInsertDestination seems to be entirely useless
>>> practically. I’m guessing it’s a relic of early development. The normal
>>> BlockPoolInsertDestination seems like a degenerate case of the
>>> PartitionAware one, with a single partition.
>>>
>>> If my understanding of the design is right, then we should refactor this
>>> code to only have one InsertDestination. There is a lot of duplication of
>>> code and complex logic in there. Anything we can do to simplify and clean
>>> up the code is probably a good move.
>>>
>>> Thoughts?
>>>
>>> Cheers,
>>> Navneet
>>>
>>>
> --
> Thanks,
> Harshad
>
>
Re: Do we need 3 different InsertDestination classes?
Posted by Harshad Deshmukh <ha...@cs.wisc.edu>.
Hi Navneet,
I was always in favor of getting rid of AlwaysCreateBlock derived class.
This issue was raised long time back, but Craig insisted that we keep it.
I guess the PartitionAware class can't be an efficient drop in
replacement for the BlockPool class. The reason is that it checks for
each input tuple, which output partition should it be sent to. This is
an unnecessary step when we don't have partitioning enabled.
An alternative is something called PartitionPreserving derived class,
that Gerald wrote. It skips the above check that I mentioned.
Gerald, please feel free to correct if I didn't get it right.
On 06/15/2016 03:20 AM, Jignesh Patel wrote:
> A huge +1 from me to get rid of dead code!
>
> +CC: Gerald who also worked on this a while back, in case he has some input.
>
> Thanks!
> Jignesh
>
>
>> On Jun 14, 2016, at 11:36 PM, Navneet Potti <na...@gmail.com> wrote:
>>
>> Hi Harshad
>> I\u2019m just kicking off this discussion based on a conversation I had with Jignesh this morning. We have 3 different implementations of InsertDestination, and I think we only really need one, the PartitionAwareBlockPoolInsertDestination.
>> The AlwaysCreateBlockInsertDestination seems to be entirely useless practically. I\u2019m guessing it\u2019s a relic of early development. The normal BlockPoolInsertDestination seems like a degenerate case of the PartitionAware one, with a single partition.
>>
>> If my understanding of the design is right, then we should refactor this code to only have one InsertDestination. There is a lot of duplication of code and complex logic in there. Anything we can do to simplify and clean up the code is probably a good move.
>>
>> Thoughts?
>>
>> Cheers,
>> Navneet
>>
--
Thanks,
Harshad
Re: Do we need 3 different InsertDestination classes?
Posted by Jignesh Patel <ji...@cs.wisc.edu>.
A huge +1 from me to get rid of dead code!
+CC: Gerald who also worked on this a while back, in case he has some input.
Thanks!
Jignesh
> On Jun 14, 2016, at 11:36 PM, Navneet Potti <na...@gmail.com> wrote:
>
> Hi Harshad
> I’m just kicking off this discussion based on a conversation I had with Jignesh this morning. We have 3 different implementations of InsertDestination, and I think we only really need one, the PartitionAwareBlockPoolInsertDestination.
> The AlwaysCreateBlockInsertDestination seems to be entirely useless practically. I’m guessing it’s a relic of early development. The normal BlockPoolInsertDestination seems like a degenerate case of the PartitionAware one, with a single partition.
>
> If my understanding of the design is right, then we should refactor this code to only have one InsertDestination. There is a lot of duplication of code and complex logic in there. Anything we can do to simplify and clean up the code is probably a good move.
>
> Thoughts?
>
> Cheers,
> Navneet
>