arjunmehta/node-georedis

Incorrect results if not specifying nativeGeo when the redis server supports native geo command

Closed this issue · 8 comments

seoker commented

Hi,

I am using redis server 3.2.3 which supports native geo command.
I found that if I call nearby() function with the following options, the results will be very limited (I guess the unit used isn't correct)

{
  withCoordinates: true,
  withDistances: true,
  units: 'mi',
  accurate: true
}

My peer uses old redis server (3.0.7), it works fine with the same code. Then I tried to append nativeGeo: true on initialization, everything goes well. But my server doesn't support native geo command, it will be great and better let the lib determine whether to use native command.

Thanks :]

arjunmehta commented

@seoker thank you so much for reporting this bug.
I will try and look into it to see how easy it will be to fix. I agree that the module should auto detect the commands. I'll see if there is a way to detect more reliably.

In the meantime, feel free to look into the code and contribute :)

  • 👍1
seoker commented

@arjunmehta
thanks for the quick reply, I'll try to contribute when I have time coz I'm currently running a startup :O
Hope this help: http://redis.io/commands/command

arjunmehta commented

I can't seem to replicate this bug. Seems like you were trying to force the use of native geo commands on a server that didn't support it?

seoker commented

Sorry, it's been a long time since I posted this issue, let me try to recap for this issue. 😂

I have a Redis server built on my machine which supports native geo command, but if I didn't append nativeGeo: true while initializing, the search result goes very abnormal (only very few results were returned while it should be much more). This is something I think is a bug.

For some scenarios we might have no idea if the server supports native geo or not, plus to reduce the efforts to configure for various environments, I then proposed if the library could help determine whether to use native geo command by default.

arjunmehta commented

@seoker Thank you for the follow up!

Right now the module does try to configure and detect whether the server supports native geo commands or not here. It's very possible this isn't doing what it should be doing.

Out of curiosity, are you adding locations right away when the module is loaded? Or is there some time before you add locations/query?

The reason why I ask, is I wonder if there are locations being added before the initial check completes.

To resolve this I may need to queue up requests before determining for certain that a server supports native commands or not.

I can try to work on this over the weekend and write a test to ensure it is working correctly.

Thank you!

  • 👍1
seoker commented

Hi,

Thanks for reopening this.
To answer your question, I inserted the locations in the downtime, far before the client get initialized.

Sorry I didn't try the latest version to see if it works now because we have migrate the geo search from this to elasticsearch. :P

arjunmehta commented

This makes sense. I'm realizing this is a major bug and it's not an easy problem to solve, but there are two things I can do:

  1. The easiest thing to do would be to have a ready or initialized event. That can be used to know when to start using the module. Not the most intuitive or convenient solution.
  2. More time consuming to do would be to have a queue that is drained on initialization with the right interface. This would be invisible and seamless. This would take some time to develop and test, but is the ideal solution (in my opinion).

Anyway, I'll give it a go soon.

Thanks!

  • 👍1
arjunmehta commented

@seoker I've address this in the latest release, 3.1.1.

Published to npm. I went with option 2 from above. A command queue is created and drained upon detection of the right interface.

  • 👍1