C++ migration: port write stream-related part of FSTRemoteStore
#2335
Conversation
| void OnWriteStreamOpen() override; | ||
|
|
||
| /** | ||
| * Handles a successful handshake response from the server, which is our cue |
There was a problem hiding this comment.
This comment seems essentially duplicated from the parent class. I think you could just eliminate it.
| /** | ||
| * Handles the closing of the StreamingWrite RPC, either because of an error | ||
| * or because the RPC has been terminated by the client or the server. | ||
| */ |
There was a problem hiding this comment.
(again, though the comment is much simpler here. I think it's still essentially the same though.)
| /** | ||
| * Handles the closing of the StreamingWrite RPC, either because of an error or | ||
| * because the RPC has been terminated by the client or the server. | ||
| */ |
There was a problem hiding this comment.
Comment duplicated from header. Recommend removing it.
| /** | ||
| * Handles a successful StreamingWriteResponse from the server that contains a | ||
| * mutation result. | ||
| */ |
There was a problem hiding this comment.
Done. This now allows grouping these overridden methods together, which I find more readable.
| // If this was a permanent error, the request itself was the problem so it's | ||
| // not going to succeed if we resend it. | ||
| FSTMutationBatch* batch = write_pipeline_.front(); | ||
| write_pipeline_.erase(write_pipeline_.begin()); |
There was a problem hiding this comment.
Here (and above) you're deleting from the front of a vector. (And adding to the end.) A std::[de]queue might be a better choice, despite locality of reference. (It also happens to be what android uses.)
There was a problem hiding this comment.
std::queue cannot be used because of the for loop at line 351. My impression is that deque starts outperforming vector only for a very large dataset (something like tens of thousands of elements). All things considered, I'm slightly in favor of the more common vector.
| @@ -48,6 +52,12 @@ | |||
| namespace firestore { | |||
| namespace remote { | |||
|
|
|||
| /** | |||
| * The maximum number of pending writes to allow. | |||
| * TODO(bjornick): Negotiate this value with the backend. | |||
There was a problem hiding this comment.
The ts port uses (still open) b/35853402 rather than bjornick.
| // If the write stream closed due to an error, invoke the error callbacks if | ||
| // there are pending writes. | ||
| if (!status.ok() && !write_pipeline_.empty()) { | ||
| if (write_stream_->handshake_complete()) { |
There was a problem hiding this comment.
Well, that doc suggests that unauth errors should trigger a retry with a refreshed token, and then permanently fail. We don't seem to be doing that, so I suspect the TODO is relevant. (Unless this is handled elsewhere...?)
If you want to keep/add the TODO, consider adding some additional context. Maybe:
/*
* TODO: handle UNAUTHENTICATED status.
* The first unauthenticated error should trigger a retry with a refreshed token. Subsequent attempts within a
* time delta should then permanently fail. See go/firestore-client-errors for details.
*/
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
CLAs look good, thanks! |
* master: (27 commits) Pass FSTMutations using a vector (#2357) Update CI to use CocoaPods 1.6.0 (#2360) Add NS_ASSUME_NONNULL_NOTATION for game center sign in (#2359) C++ migration: make all methods of `FSTRemoteStore` delegate to C++ (#2337) C++ migration: port write stream-related part of `FSTRemoteStore` (#2335) Resolve hard dependency of GameKit (#2355) Update gRPC certificate bundles locations for Firebase 5.16 (#2353) Start pod lib lint CI for IAM (#2347) Update library name to `fire-fiam`. (#2352) Bump FirebaseAnalyticsInterop version (#2315) Add IAM headless to CI (#2341) C++ migration: port watch stream-related part of `FSTRemoteStore` (#2331) Open source FIAM headless SDK (#2312) Port flaky test fix from web. (#2332) Make scripts/style.sh compatible with newer swiftformat versions (0.38) (#2334) C++ migration: port `FSTOnlineStateTracker` (#2325) Don't build fuzz tests target on Travis (#2330) Remove FSTMutationQueue (#2319) C++ migration: port `FSTRemoteEvent` (#2320) C++ migration: port `FSTTargetChange` (#2318) ...
No description provided.