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

Pending Reads not completing #378

Closed
ThomasSchuh opened this issue Nov 25, 2020 · 5 comments · Fixed by #380
Closed

Pending Reads not completing #378

ThomasSchuh opened this issue Nov 25, 2020 · 5 comments · Fixed by #380

Comments

@ThomasSchuh
Copy link

Hi there,

I'm having the issue that Read() on keys that have been written to disk give me Status.Pending when i try to read them again. Tested a bunch already and had a look at the implementations in your unit tests and benchmark project but i can't see what do wrong. I actually tried to set up the benchmark project similar to mine and ran into the same issue.

Tried closing the session after the writes and crating a new one before reading, as well as session.CompletePending() and
session.Refresh() but nothing appear to impact my case at all. Also saw context.FinalizeRead() somewhere in the tests so i set that up with the same result.

In the serializers Deserialize() call i seem to get an empty stream for those items, not sure why that is (and if it happens consistently. Didn't follow up on that detail).

I pushed my test project here. It will Upsert a bunch of items and then tries to read them. From what i saw i'd say it can consistently read what is still in memory but all other items are stuck on pending.

Would be awesome if you could have a quick look and point me in the right direction.

Regards,
Thomas

@badrishc
Copy link
Contributor

The way FASTER returns pending results, when using the sync API, is via completion callbacks. These are provided via IFunctions argument to NewSession. You are using the default SimpleFunctions<CacheKey, CacheValue, StoreContext> which do nothing in the callback. You need to override this class and provide your own callback as follows:

public class StoreFunctions : SimpleFunctions<CacheKey, CacheValue, StoreContext>
{
   public override void ReadCompletionCallback(ref CacheKey key, ref CacheValue input, ref CacheValue output, StoreContext ctx, Status status)
      => ctx.Populate(ref status, ref output);
}

In the above callback, we simply populate the context with the status and output.

You can then instantiate the session as follows:

var session = _fasterStore.NewSession(new StoreFunctions());

@badrishc
Copy link
Contributor

The other option would be to use the async Read API: ReadAsync. See example here.

@ThomasSchuh
Copy link
Author

Thanks a lot for the quick response. Really appreciate it.

I could solve the "Pending" issue with the ReadCompletionCallback. But that led me back to my inital problem which is that the Serializer at some point gets an empty stream. I played around with that the whole day and from my observations I'd say I get the empty stream once I read a Page worth of data from disk. Unfortunately I don't really understand what impact the Page size should have so I'm kind of guessing here.

I updated the example repo. With that setup i quickly run into the empty stream issue a few times followed by a way to big stream. Thought it might be just to small pages but making them bigger only delays the issue. At some point I always get empty streams.

@badrishc
Copy link
Contributor

badrishc commented Nov 27, 2020

The FASTER page size has to be at least of one sector size (which is 512 bytes). That means, PageSizeBits has to be set to 9 or more. Updating your testcase with PageSizeBits of 9 and MemorySizeBits of 10 runs to completion without errors. We have linked a PR that will provide an error message if users attempt to use a smaller page size.

Also, these are really small values in general. I would recommend a PageSizeBits of at least 20 (1MB) and MemorySizeBits of at least 26 (64MB) for decent performance.

See here for more tuning information.

@ThomasSchuh
Copy link
Author

I see, will give it a try. Thanks!

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 a pull request may close this issue.

2 participants