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

Port FSTSyncEngine to C++ SyncEngine 1/2 #3726

Merged
merged 8 commits into from
Sep 4, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions Firestore/Source/Core/FSTSyncEngine.mm
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,12 @@ - (void)transactionWithRetries:(int)retries
workerQueue:(const std::shared_ptr<AsyncQueue> &)workerQueue
updateCallback:(core::TransactionUpdateCallback)updateCallback
resultCallback:(core::TransactionResultCallback)resultCallback {
_syncEngine->Transaction(retries, workerQueue, updateCallback, resultCallback);
_syncEngine->Transaction(retries, workerQueue, std::move(updateCallback),
std::move(resultCallback));
}

- (void)applyRemoteEvent:(const RemoteEvent &)remoteEvent {
_syncEngine->HandleRemoteEvent(remoteEvent);
_syncEngine->ApplyRemoteEvent(remoteEvent);
}

- (void)applyChangedOnlineState:(OnlineState)onlineState {
Expand All @@ -186,7 +187,7 @@ - (void)rejectFailedWriteWithBatchID:(BatchId)batchID error:(NSError *)error {
return _syncEngine->GetCurrentLimboDocuments();
}

- (void)credentialDidChangeWithUser:(const firebase::firestore::auth::User &)user {
- (void)credentialDidChangeWithUser:(const User &)user {
_syncEngine->HandleCredentialChange(user);
}
- (DocumentKeySet)remoteKeysForTarget:(TargetId)targetId {
Expand Down
41 changes: 21 additions & 20 deletions Firestore/core/src/firebase/firestore/core/sync_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_CORE_SYNC_ENGINE_H_
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_CORE_SYNC_ENGINE_H_

#if !defined(__OBJC__)
#error "This header only supports Objective-C++"
#endif // !defined(__OBJC__)

#include <map>
#include <memory>
#include <string>
Expand All @@ -36,7 +40,6 @@
#include "Firestore/core/src/firebase/firestore/model/document_key_set.h"
#include "Firestore/core/src/firebase/firestore/model/maybe_document.h"
#include "Firestore/core/src/firebase/firestore/remote/remote_store.h"
#include "Firestore/core/src/firebase/firestore/util/delayed_constructor.h"
#include "Firestore/core/src/firebase/firestore/util/status.h"

namespace firebase {
Expand All @@ -61,14 +64,14 @@ class SyncEngine {
public:
SyncEngine(FSTLocalStore* local_store,
remote::RemoteStore* remote_store,
const auth::User initial_user);
const auth::User& initial_user);

void SetCallback(SyncEngineCallback* callback) {
sync_engine_callback_ = callback;
}

/**
* Initiates a new listen. The FSTLocalStore will be queried for initial data
* Initiates a new listen. The LocalStore will be queried for initial data
* and the listen will be sent to the `RemoteStore` to get remote data. The
* registered SyncEngineCallback will be notified of resulting view
* snapshots and/or listen errors.
Expand Down Expand Up @@ -115,16 +118,17 @@ class SyncEngine {
void HandleCredentialChange(const auth::User& user);

// Implements `RemoteStoreCallback`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is comment refers to something coming up in the second PR. Please ignore for now.

void HandleRemoteEvent(const remote::RemoteEvent& remote_event);
void ApplyRemoteEvent(const remote::RemoteEvent& remote_event);
void HandleRejectedListen(model::TargetId target_id, util::Status error);
void HandleSuccessfulWrite(const model::MutationBatchResult& batch_result);
void HandleRejectedWrite(firebase::firestore::model::BatchId batchID,
util::Status error);
void HandleOnlineStateChange(model::OnlineState online_state);
model::DocumentKeySet GetRemoteKeys(model::TargetId targetId);
model::DocumentKeySet GetRemoteKeys(model::TargetId targetId) const;

// For tests only
std::map<model::DocumentKey, model::TargetId> GetCurrentLimboDocuments() {
std::map<model::DocumentKey, model::TargetId> GetCurrentLimboDocuments()
const {
// Return defensive copy
return limbo_targets_by_key_;
}
Expand All @@ -146,15 +150,15 @@ class SyncEngine {
view_(std::move(view)) {
}

const Query& query() {
const Query& query() const {
return query_;
}

/**
* The target ID created by the client that is used in the watch stream to
* identify this query.
*/
model::TargetId target_id() {
model::TargetId target_id() const {
return target_id_;
}

Expand All @@ -163,7 +167,7 @@ class SyncEngine {
* the results that was received. This can be used to indicate where to
* continue receiving new doc changes for the query.
*/
const nanopb::ByteString resume_token() {
const nanopb::ByteString resume_token() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return either a non-const value or a const reference (probably the latter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return resume_token_;
}

Expand All @@ -173,8 +177,8 @@ class SyncEngine {
* applies the query filters and limits to determine the most correct
* possible results.
*/
View* view() {
return &view_;
View& view() {
return view_;
}

private:
Expand All @@ -187,8 +191,7 @@ class SyncEngine {
/** Tracks a limbo resolution. */
class LimboResolution {
public:
LimboResolution() {
}
LimboResolution() = default;

explicit LimboResolution(const model::DocumentKey& key) : key{key} {
}
Expand All @@ -204,7 +207,7 @@ class SyncEngine {
bool document_received = false;
};

void AssertCallbackExists(std::string source);
void AssertCallbackExists(absl::string_view source);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add the appropriate include (I think it's third_party/absl/strings/string_view.h).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


ViewSnapshot InitializeViewAndComputeSnapshot(
const local::QueryData& query_data);
Expand All @@ -217,7 +220,7 @@ class SyncEngine {
const model::MaybeDocumentMap& changes,
const absl::optional<remote::RemoteEvent>& maybe_remote_event);

/** Updates the limbo document state for the given targetID. */
/** Updates the limbo document state for the given target_id. */
void UpdateTrackedLimboDocuments(
const std::vector<LimboDocumentChange>& limbo_changes,
model::TargetId target_id);
Expand All @@ -233,24 +236,22 @@ class SyncEngine {
void TriggerPendingWriteCallbacks(model::BatchId batch_id);
void FailOutstandingPendingWriteCallbacks(absl::string_view message);

bool ErrorIsInteresting(util::Status error);

/** The local store, used to persist mutations and cached documents. */
FSTLocalStore* local_store_;

/** The remote store for sending writes, watches, etc. to the backend. */
remote::RemoteStore* remote_store_;
remote::RemoteStore* remote_store_ = nullptr;

auth::User current_user_;
SyncEngineCallback* sync_engine_callback_;
SyncEngineCallback* sync_engine_callback_ = nullptr;

/**
* Used for creating the TargetId for the listens used to resolve limbo
* documents.
*/
TargetIdGenerator target_id_generator_;

/** Stores user completion blocks, indexed by user and BatchId. */
/** Stores user completion blocks, indexed by User and BatchId. */
std::unordered_map<auth::User,
std::unordered_map<model::BatchId, util::StatusCallback>,
auth::HashUser>
Expand Down
Loading