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

Add store test case for GeoRadiusByMember #2210

Merged
merged 6 commits into from
Nov 27, 2023

Conversation

azuredream
Copy link
Contributor

@azuredream
Copy link
Contributor Author

Please refer to PR2149 for the history.
I'm still seeing the same issue, store worked in CLI but not in test:

I encountered a wired issue. This command works in CLI:
127.0.0.1:6379> GEORADIUSBYMEMBER Europe Madrid 700 KM STORE test1
(integer) 2
(15.54s)
127.0.0.1:6379> zrange test1 0 -1

  1. "Madrid"
  2. "Lisbon"

The first unit test case worked as well.
GEORADIUSBYMEMBER Europe Madrid 700 KM WITHCOORD WITHDIST

However, the second test case failed. Please note that this command worked in CLI.
GEORADIUSBYMEMBER Europe Madrid 700 KM STORE store_key
[ RUN ] ZSetFamilyTest.GeoRadiusByMember
get resp: ERR Member not found
F20231114 21:37:55.006270 2607605 test_utils.cc:466] Check failed: RespExpr::STRING == int(resp.type) (0 vs. 6) [GEORADIUSBYMEMBER, Europe, Madrid, 700, KM, STORE, store_key]

When I used GDB to run the test case line by line, it worked again.

I suppose it's something to do with the schedule-execute model.
When I use STORE option, there will be 3 transactions while 2 without STORE.
It's confusing to me that the test case failed at the first transaction (get key from db), not the last one.
I'd appreciate it if you could share your thought on this. Thanks!

bool MembersOfAllNeighbors(ConnectionContext* cntx, string_view key, const GeoHashRadius& n,
const GeoShape& shape_ref, GeoArray* ga, unsigned long limit) {
const GeoShape& shape_ref, GeoArray* ga, unsigned long limit,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can it be that you call MembersOfAllNeighbors(...,, true, false) ?

if (result.status() == OpStatus::WRONG_TYPE) {
cntx->transaction->Schedule();
cntx->transaction->Execute(std::move(cb), false);
if (member_score.status() == OpStatus::WRONG_TYPE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can not call Execute(..) and then exit early. In fact, please a test that runs this command on a wrong type and see the testing framework fails because of this early return.

If you want to return early you should call transaction->Conclude();

@azuredream azuredream marked this pull request as ready for review November 24, 2023 19:23
@azuredream
Copy link
Contributor Author

azuredream commented Nov 24, 2023

Hi romange, I couldn't make STORE and STOREDIST work shortly. Schedule-execute and sharding are confusing to me. I'll have to learn more about the source code. A guess is it that I should have used ScheduleSingleHopT instead of schedule-execute. But more issue came out. Maybe someone else knows how to write DB correctly.

Just updated my PR to add test cases and fixed a bug in GeoSearch.

romange
romange previously approved these changes Nov 26, 2023
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@romange
Copy link
Collaborator

romange commented Nov 26, 2023

@azuredream I see you reverted the parsing code as well, is it intended?

@azuredream
Copy link
Contributor Author

I just put the parsing code back. Thanks for the reminder

Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

Thank you!

@romange romange merged commit 3f7e42b into dragonflydb:main Nov 27, 2023
7 checks passed
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.

None yet

2 participants