-
Notifications
You must be signed in to change notification settings - Fork 810
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
docstore/awsdynamodb panics on iter.Next() #3405
Comments
This is hard to debug from here, can you use gohack and add some Printfs in the relevant code to see what's happening? In here: https://github.com/google/go-cloud/blob/master/docstore/awsdynamodb/query.go#L472 Specifically it would be good to know what I think that this line: https://github.com/google/go-cloud/blob/master/docstore/awsdynamodb/query.go#L480 is supposed to handle this, but it's not. It's possible that this is a regression in #3385. @bartventer FYI |
I've been digging into this issue and although I was unable to reproduce the panic, I added a preventive check for I also added a test case to verify this behavior. The test repeatedly calls I've pushed these changes in a PR. Please have a look when you get a chance and let me know if there's anything that needs adjusting. |
Thanks for the fix. I can confirm that I am no longer seeing panics calling https://github.com/sfomuseum/go-activitypub/blob/iter-fix/go.mod#L5 Question: In the process of investigating the panics I often noticed that the
All the query iterators return nothing. But then if I call the same code immediate afterwards the queries return results:
The code in question is:
I am uncertain whether this is related to the |
Are you using |
and/or |
@bartventer I think there's a bug in Example:
|
I am not currently using either |
Hrm that damages my theory, but I still think there's a bug in those. @thisisaaronland it would be really helpful if you could provide some debugging info as requested above, as it seems like you can reproduce this easily and we can't. Maybe even comment out the new lines that fixed the panic so we can understand what's going on there too. |
Sure thing. I may not be able to do that until tomorrow but I will get to it as soon as I can. Thanks for all the help so far. |
Per AWS, "A Query operation can return an empty result set and a LastEvaluatedKey if all the items read for the page of results are filtered out." I think we're not handling that correctly, which causes the empty results sometimes, as well as the panic. @bartventer can you take a look at #3408 ? @thisisaaronland can you try it out and see if that looks better? |
I couldn't figure out what the
I will carve out some time in the morning to add the debugging hooks you suggested (above) to try and get more concrete details to work with. If it's at all helpful what I have observed is that the |
Can you try again, I made some changes. If it doesn't work let me know,
printfs...
…On Thu, Mar 21, 2024, 6:16 PM Aaron Straup Cope ***@***.***> wrote:
I couldn't figure out what the replace directive for the changes in #3408
<#3408> should be so I just
brute-force cloned the query.go file in my vendor folder. With only
limited testing it doesn't seem to have helped and may, in fact, have made
things worse. Specifically:
- Running the code (mentioned above) for the first time the Query
iterator continues to return no results.
- Running the code (mentioned above) a second time simply hung and
never returned any results.
I will carve out some time in the morning to add the debugging hooks you
suggested (above) to try and get more concrete details to work with.
If it's at all helpful what I have observed is that the Query iterator
seems to return no results the first time it is invoked but then
subsequently it starts to return results, even though the queries
themselves are happening in entirely new processes. So maybe there is
something happening at the AWS/Dynamo layer where the iterator returns
prematurely on the first invocation and that, by extension, Dynamo queries
exist in some kind of "window of time" between invocations ?
—
Reply to this email directly, view it on GitHub
<#3405 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFMCKTSPUUQCJOERCSE74TLYZOA55AVCNFSM6AAAAABE4FMXTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJUGEZTSOJVHA>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
I've had a look at PR #3408 and I agree with the logic there. I've created a new PR (#3409) that builds on that work. @vangent, could you take a look when you get a chance? |
Just a quick note that I applied https://github.com/sfomuseum/go-activitypub/blob/iter-fix/go.mod#L7 Specifically: The first time the query is run it will sometimes return 0 results but then if the query is run again inside a short window of time (but in an entirely new process) the query will return results. That's all I've been able to do so far but will try to carve out time to get more concrete debugging information soon. |
Ping @thisisaaronland |
Any updates here @thisisaaronland ? |
Describe the bug
Calling
iter.Next
on adocstore/awsdynamodb
derivedQuery
instance panics with an "index out of range" error along the lines of:The
Query
itself has two "Where" statements bracketing a date range:https://github.com/sfomuseum/go-activitypub/blob/main/accounts_database_docstore.go#L45-L69
Curiously, this problem only seems to affect one table (accounts). The same code (essentially) run against a different table (deliveries) does not panic:
https://github.com/sfomuseum/go-activitypub/blob/main/deliveries_database_docstore.go#L46-L58
To Reproduce
The code in question is here:
https://github.com/sfomuseum/go-activitypub/blob/main/cmd/counts-for-date/main.go
Specifically:
https://github.com/sfomuseum/go-activitypub/blob/main/stats/counts.go#L44
Which is run like this:
If you need/want to set up all those tables, the code to do so is here:
https://github.com/sfomuseum/go-activitypub/blob/main/cmd/create-dynamodb-tables/main.go
And:
https://github.com/sfomuseum/go-activitypub/tree/main/app/dynamodb/tables/create
For example:
Note that all of these URIs are using a custom
awsdynamodb://
scheme. Under the hood all of this code is using theaaronland/go-aws-dynamodb
andaaronland/go-aws-session
packages in to support a wider range of declarative credentialing options than those offered by the default AWS libraries:There are also
Makefile
targets for running everything locally against a containerizer DynamoDB instance:Expected behavior
An iterator whose
Next
method triggers anio.EOF
error.Version
0.37.0
Additional context
Documentation (albeit incomplete) for the application's database design and specifics about the use of
gocloud.dev/docstore
are here:The really long think-y think-y version is here:
The text was updated successfully, but these errors were encountered: