Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Processing some queries in batches and new EXPLAIN command #318

Closed
wants to merge 11 commits into from

Conversation

elcct
Copy link
Contributor

@elcct elcct commented Mar 9, 2014

See my comments for #294 and #263

@@ -19,6 +19,10 @@ import (
"time"
)

const (
POINT_BATCH_SIZE = 256
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be great to move this into a configuration option as you mentioned before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added it.

It is now point-batch-size under [leveldb] section in config.toml

@jvshahid jvshahid added this to the 0.5.0 milestone Mar 10, 2014
@jvshahid
Copy link
Contributor

Thanks @elcct for your contribution. We're really excited about the new features that you added in this pull request. Couple of things that will make it extremely easy for us to merge your changes in the future (I'll put these guidelines in a commiters readme file):

  1. Add tests for the changes you're putting in. This will make it easy for us to review the changes and avoid regressions in the future. It's ok for this pr, i'll just add the test
  2. Each pull request should contain one feature or bug fix, in this case you have the explain query and partial fix for Engine interface should change to take batches of points #294 combined in the same pr.
  3. Commits should be squashed, each feature or bug fix should go in one commit to make it easier to view the history of the project and revert changes (not that we'll every want to do that)
  4. You should open pull requests from a branch not from master. Otherwise changes that go in your repo's master will show up in old pull requests that you opened.
  5. Normally we'd ask contributors to rebase their branches on top of current master. In this case, I felt master has so many changes that it'd be more appropriate if we did the rebase. See https://github.com/influxdb/influxdb/tree/pr-318 for the squashed commits of the two features that were on your branch.

If you have any questions or comments feel free to tell us.

Cheers,

@jvshahid jvshahid modified the milestones: 0.6.0, 0.5.0 Mar 10, 2014
@elcct
Copy link
Contributor Author

elcct commented Mar 10, 2014

Thank you. I am sorry for the trouble, next time I will follow those guidelines.

@jvshahid jvshahid closed this in e2adcf1 Mar 13, 2014
@jvshahid
Copy link
Contributor

I merged my branch which included some tests and bug fixes. I encourage you to take a look and verify that I didn't do something stupid. doing git diff dbe76df..7bda031 should give you the changes that i just merged into master.

Thanks again for your contribution.

@elcct
Copy link
Contributor Author

elcct commented Mar 14, 2014

Great, i should take a look during the weekend.

@jvshahid jvshahid modified the milestones: 0.5.0, 0.6.0 Apr 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants