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

Added method to add multiple locations to GeoFire at once #56

Merged
merged 4 commits into from
Nov 13, 2014

Conversation

jwngr
Copy link

@jwngr jwngr commented Nov 13, 2014

@jdimond - Writing to GeoFire is making our transit Open Data Set practically unusable. We end up writing to the Firebase hundreds of times a second because we can only add one location to GeoFire at a time. This PR adds a new batchSet() method which allows you to add as many locations to GeoFire in only one write. I made it backwards compatible with the previous GeoFire version and data structure.

This should significantly improve the performance of our Open Data Set and make it usable once again. Let's sync up on this tomorrow and talk about porting this to the other clients.

cc/ @tonymeng @vikrum

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) when pulling 2b98ef8 on jw-batch-set into 364da34 on master.

@tonymeng
Copy link

nice turn around time, let me know when this goes out and i'll fix transit

@@ -428,6 +428,7 @@ function encodeGeoFireObject(location, geohash) {
validateLocation(location);
validateGeohash(geohash);
return {
".priority": geohash,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this documented behavior? I couldn't find anything in the docs (but might have missed it)

Copy link
Author

Choose a reason for hiding this comment

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

This is documented for our REST API and I would not consider it a "hack" by any means. It is a part of our API, albeit not a very well documented one. I doubt we will add documentation around this since we plan to phase out priorities.

@jdimond
Copy link
Contributor

jdimond commented Nov 13, 2014

@jwngr lgtm except the two issues I mentioned. Also I would prefer overloading set, since that seems to be the standard js way of doing things, than adding a new method batchSet, but don't feel super strongly though

@jwngr
Copy link
Author

jwngr commented Nov 13, 2014

Back to you @jdimond.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.11%) when pulling 935dbde on jw-batch-set into 364da34 on master.

@jwngr
Copy link
Author

jwngr commented Nov 13, 2014

Back to you once more @jdimond. Thanks for the code review!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.13%) when pulling 627cf8e on jw-batch-set into 364da34 on master.

jdimond added a commit that referenced this pull request Nov 13, 2014
Added method to add multiple locations to GeoFire at once
@jdimond jdimond merged commit 70be45c into master Nov 13, 2014
@jwngr jwngr deleted the jw-batch-set branch November 13, 2014 23:46
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.

4 participants