You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by navsan <gi...@git.apache.org> on 2016/06/27 18:57:04 UTC
[GitHub] incubator-quickstep pull request #43: Remove unused AlwaysCreateBlockInsertD...
GitHub user navsan opened a pull request:
https://github.com/apache/incubator-quickstep/pull/43
Remove unused AlwaysCreateBlockInsertDestination
As per [discussion](http://mail-archives.apache.org/mod_mbox/incubator-quickstep-dev/201606.mbox/%3CCAA-fBsovOVutKi%3DfvTpQ8FMW%3DTf8rSSj8H-3JB7QHGXuYFkVyA%40mail.gmail.com%3E) with @adalbertgerald and @hbdeshmukh in the dev mailing list.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep remove_alwayscreateinsertdest
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/43.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #43
----
commit dab473f40f538fb130338b034c642f9dc87d1306
Author: Navneet Potti <na...@gmail.com>
Date: 2016-06-27T16:22:47Z
Remove unused AlwaysCreateBlockInsertDestination
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
@zuyu Any objections to my proposal [above](https://github.com/apache/incubator-quickstep/pull/43#issuecomment-231207534)? Can I go ahead with the submission?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
Hi @hbdeshmukh, I've made the corresponding change to the .proto file.
On the `always_mark_full` flag: I agree that it doesn't seem to be used anywhere. But is it directly related to the `AlwaysCreateBlockInsertDestination`? I didn't see any comments explaining why this flag was added, and the flag is part of the `InsertDestination` interface. Just to be safe, I've made the change in a separate commit, rather than squashing it.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
@navsan Hold, we may need it for the long-standing streaming queries.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
i\u2019m sorry, I don\u2019t quite follow that. Would you mind elaborating?
> On Jul 9, 2016, at 01:46, Zuyu ZHANG <no...@github.com> wrote:
>
> @navsan <https://github.com/navsan> No, we have to close this PR w/o merging, because we still need it as a workaround of lacking the concurrent control over multiple update queries.
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-quickstep/pull/43#issuecomment-231519184>, or mute the thread <https://github.com/notifications/unsubscribe/ACZB6xGHlEmvnR55IpxzN_2YQhmqIT8xks5qT0O2gaJpZM4I_ZeS>.
>
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
@navsan No, we have to close this PR w/o merging, because we still need it as a workaround of lacking the concurrent control over multiple update queries.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
Hi @navsan
We need to update the storage/InsertDestination.proto to reflect the removal of this derived class.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
Do we need the optional boolean parameter ``always_mark_full = false`` in the ``bulkInsert*`` methods now? I don't see it being set to ``true`` anywhere.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
@hbdeshmukh We were wrong: `always_mark_full` is indeed being used [in the sort function](https://github.com/apache/incubator-quickstep/blob/master/storage/StorageBlock.cpp#L940). I'm not sure why it's necessary there, but I'd rather not go change that now. I'm reverting that commit and only submitting the removal of the AlwaysCreateBlockInsertDestination (first commit).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
@navsan: Can we resolve this PR? Perhaps, pull back on this? Thanks!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #43: Remove unused AlwaysCreateBlockInsertDestinat...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/43
Why? Why not just return blocks to the storage manager marking them as full? There should be negligible overhead in that case (2 branch instructions that are always predicted false; once for every block/work order).
> On Jul 7, 2016, at 16:02, Zuyu ZHANG <no...@github.com> wrote:
>
> @navsan <https://github.com/navsan> Hold, we may need it for the long-standing streaming queries.
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-quickstep/pull/43#issuecomment-231206334>, or mute the thread <https://github.com/notifications/unsubscribe/ACZB69gaUQRTNKoQzuuBi5-a645MvGMCks5qTWlSgaJpZM4I_ZeS>.
>
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---