From 35197d6c4bdccf37c210591fbcc1746a67a85105 Mon Sep 17 00:00:00 2001
From: florijan <florijan@memgraph.io>
Date: Mon, 22 Jan 2018 14:24:46 +0100
Subject: [PATCH] Add distributed record TODOs

Summary:
It seems that RecordAccessor &co are ready for read-only distributed
execution. In read-only there is no command advancement and the implied
cache invalidation, `SwitchOld` and `SwitchNew` perform default
switching and `Reconstruct` uses the `RemoteCache` which is implemented.

I just added a few TODOs for proper CRUD.

Reviewers: dgleich

Reviewed By: dgleich

Differential Revision: https://phabricator.memgraph.io/D1125
---
 src/distributed/remote_cache.hpp | 17 +++++++++++++++--
 src/storage/record_accessor.cpp  | 12 +++++-------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/src/distributed/remote_cache.hpp b/src/distributed/remote_cache.hpp
index e24647ac6..2a287e1cc 100644
--- a/src/distributed/remote_cache.hpp
+++ b/src/distributed/remote_cache.hpp
@@ -60,8 +60,10 @@ class RemoteCache {
     if (found == cache_.end()) {
       rec_uptr old_record =
           remote_data_clients_.RemoteElement<TRecord>(worker_id, tx_id, gid);
-      found = cache_.emplace(
-          gid, std::make_pair<rec_uptr, rec_uptr>(nullptr, nullptr)).first;
+      found = cache_
+                  .emplace(gid,
+                           std::make_pair<rec_uptr, rec_uptr>(nullptr, nullptr))
+                  .first;
       found->second.first.swap(old_record);
     }
 
@@ -69,6 +71,17 @@ class RemoteCache {
     new_record = found->second.second.get();
   }
 
+  void AdvanceCommand() {
+    // TODO implement.
+    // The effect of this should be that the next call to FindSetOldNew will do
+    // an RPC and not use the cached stuff.
+    //
+    // Not sure if it's OK to just flush the cache? I *think* that after a
+    // global advance-command, all the existing RecordAccessors will be calling
+    // Reconstruct, so perhaps just flushing is the correct sollution, even
+    // though we'll have pointers to nothing.
+  }
+
  private:
   std::mutex lock_;
   distributed::RemoteDataRpcClients &remote_data_clients_;
diff --git a/src/storage/record_accessor.cpp b/src/storage/record_accessor.cpp
index 1dc8a4783..d8be931e3 100644
--- a/src/storage/record_accessor.cpp
+++ b/src/storage/record_accessor.cpp
@@ -124,13 +124,8 @@ RecordAccessor<TRecord> &RecordAccessor<TRecord>::SwitchNew() {
             << "RecordAccessor::SwitchNew - accessor invalid after Reconstruct";
     }
   } else {
-    // TODO If we have distributed execution, here it's necessary to load the
-    // data from the it's home worker. When only storage is distributed, it's
-    // enough just to switch to the new record if we have it.
-    if (!new_) {
-      new_ = db_accessor().template remote_elements<TRecord>().FindNew(
-          address_.global_id(), false);
-    }
+    // A remote record only sees local updates, until the command is advanced.
+    // So this does nothing, as the old/new switch happens below.
   }
   current_ = new_ ? new_ : old_;
   return *this;
@@ -147,6 +142,9 @@ bool RecordAccessor<TRecord>::Reconstruct() const {
   if (is_local()) {
     address_.local()->find_set_old_new(db_accessor_->transaction(), old_, new_);
   } else {
+    // TODO in write queries it's possible the command has been advanced and we
+    // need to invalidate the RemoteCache and really get the latest stuff. But
+    // only do that after the command has been advanced.
     db_accessor().template remote_elements<TRecord>().FindSetOldNew(
         db_accessor().transaction().id_, address_.worker_id(),
         address_.global_id(), old_, new_);