Store
Store copied to clipboard
nytimes
Make readAll() resilient to exceptions reading a single file
Currently, if an exception occurs on a fileSystem.read(s) call inside the FSAllReader.readAll() method, then the whole chain will stop with an onError emission. This prevents us from getting subsequent files on the path.
This PR proposes a solution to flat map each individual file read, wrapping its observable. If an exception occurs for a file read, then we return a ReadResultBufferedSource for that file that wraps the exception.
Current known issues:
- A
ReadResultBufferedSourceseems like an unnecessary wrapping of a buffer just to fit the contract of theObservable<BufferedSource> readAll()method, which requires aBufferedSource. Ideally, we could return anObservable<ReadResult>instead which wraps either the exception or theBufferedSource. This would be, however, an API breaking change. - Propagating an empty buffer down the chain will cause existing parsers (e.g.,
GsonSourceParser) to returnnullfor an empty buffer. This will throw a NPE on the chain instead of the original exception when reading the file. - We could have a
safeReadAll()that filters out all the results that came from an exception?
This PR fixes #293
Ideally I'd prefer the safeReadAll as another path of execution with a way to configure when instantiating the persister. We can then deprecate readAll and remove it in the next major version
@digitalbuddha I updated the PR, adding a new method, safeReadAll() to the DiskAllRead<Raw> interface. Since I added this new method, I had to add a generic ReadResult<Raw> class to the base module as well. Hope it's clearer.
So sorry for delay. I wasn't clear with last request. What still needs to be done is adding a builder configuration for when you build the store. This configuration would be to either use the safe read all or the regular one. You would also need to change RealInternalStore to read the flag and properly call the correct persister. Alternatively you can make a new persister who's readAll is the safe version and then pass that persister in. Whichever way you go pls include a test showing usage.
@digitalbuddha Apologies for the delay in replying. I'm finally having the time to go back to this but it's not quite clear to me what you mean.
As far as I understood the code, there is currently no explicit builder configuration for any of the AllPersister implementations. In fact, as far as I can tell, the Store doesn't even use the current readAll() method at all. It doesn't seem to make sense to me to add a builder config for this.
Unless in your comment, did you mean to also add a kind of getAll() to the Store?
hey there @riclage i'm so sorry we dropped the ball on this, i am planning a holiday here shortly and will be doing some store work. i will pick this up where you left off and add to this pr if that is ok with you? i'm looking forwards to getting this in. thank you!
Hi @brianPlummer , that's ok with me. However, I feel like this PR could be merged as is. Your contributions could be made in subsequent PRs. Do you have any particular reasons to make commits to this PR specifically? Thanks!