StateDB Cache by Corey-Zumar · Pull Request #350 · ucbrise/clipper

Corey-Zumar · 2018-01-02T08:21:19Z

Fixes #329

AmplabJenkins · 2018-01-02T08:25:11Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/818/
Test FAILed.

dcrankshaw · 2018-01-03T17:36:16Z

jenkins test this please

dcrankshaw

This looks pretty good. It doesn't look like you ever evict things from the cache. Is that intentional?

dcrankshaw · 2018-01-03T17:42:14Z

src/libclipper/src/persistent_state.cpp

+    std::string redis_key = generate_redis_key(key);
+    const std::vector<std::string> cmd_vec{"GET", redis_key};
+    auto result =
+        redis::send_cmd_with_reply<std::string>(redis_connection_, cmd_vec);


If you fetch the value from Redis, you should cache it here as well.

dcrankshaw · 2018-01-03T17:54:38Z

src/libclipper/src/persistent_state.cpp

+  auto entry_search = cache_.find(key);
+  bool new_entry = (entry_search == cache_.end());
+  if (!new_entry) {
+    std::lock_guard<std::mutex> lock(entry_search->second->entry_mtx_);


What's the point of this lock? It will be released once it goes out of scope, so it won't be held after the if-statement.

dcrankshaw · 2018-01-03T17:57:32Z

src/libclipper/src/persistent_state.cpp

+      // This method is being invoked as part of the 'add application'
+      // procedure in the query frontend. Because any subsequent feedback
+      // updates to the StateDB depend on the completion of 'add application',
+      // it's safe to add a new cache entry immediately


Hmm will this always be true? This seems like a complicated behavior to rely on for safety. What if another code path ends up putting things in the StateDB? Will that break this code?

Ahh I understand your comment now. Can you clarify in the comment that if we are creating a new entry, it means that the method is being invoked as part of adding an application?

But my previous question still remains, this seems like tricky behavior to rely on for safety.

AmplabJenkins · 2018-01-03T18:29:04Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/819/
Test PASSed.

Corey-Zumar · 2018-01-21T23:19:14Z

@dcrankshaw A couple questions regarding performance vs consistency tradeoffs:

In order to ensure strong consistency, it's necessary to update the cache and database as a single atomic operation. Therefore, writers need to hold a lock on both the cache and redox handle until the write is complete. This seems fine if most StateDB operations are reads and we don't frequently evict elements (this is currently the case). However, if the distribution of reads and writes becomes more balanced and we intend to evict elements, we may not see much of an improvement in performance by adding the cache. Can we make any assumptions about StateDB access patterns going forward?
Another option is to use a weaker consistency model. If we take a lock on the redox handle during a write, we can ensure consistent ordering between writers. However, readers will continue to read old values from the cache until the thread that completed the Redox write operation is able to write the new cache value. This model will perform significantly faster in the case of simultaneous reads and writes. Does this sound like a better approach?

This reverts commit 2280e14.

Corey-Zumar · 2018-01-28T05:09:58Z

@dcrankshaw I redesigned the put and get procedures to meet the following requirements:

Write operations are serialized
Read operation results reflect all prior writes (they may also reflect subsequent writes)

This design no longer makes any assumptions about the origin of the first put call.

AmplabJenkins · 2018-01-28T05:17:19Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/912/
Test FAILed.

AmplabJenkins · 2018-01-28T06:33:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/914/
Test FAILed.

Corey-Zumar · 2018-01-28T21:15:54Z

jenkins test this please

AmplabJenkins · 2018-01-28T21:46:54Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/917/
Test FAILed.

dcrankshaw

LGTM. Will merge once tests pass.

AmplabJenkins · 2018-02-04T21:37:42Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/935/
Test FAILed.

AmplabJenkins · 2018-02-05T04:22:22Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/945/
Test FAILed.

AmplabJenkins · 2018-02-07T18:31:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/969/
Test PASSed.

Corey-Zumar added 6 commits December 30, 2017 02:15

StateDB cache - needs tests

506330c

Add test

ac2dc8f

Format code

3744b9e

atomic attempt

a3ddec4

Finish implementation

d9efc06

Revert formatted rpc, redis files

e115af9

Corey-Zumar added status: needs review type: enhancement labels Jan 2, 2018

Corey-Zumar requested a review from dcrankshaw January 2, 2018 08:21

Corey-Zumar added type: performance and removed type: enhancement labels Jan 2, 2018

dcrankshaw requested changes Jan 3, 2018

View reviewed changes

dcrankshaw added status: needs revision and removed status: needs review labels Jan 3, 2018

Merge remote-tracking branch 'ucbrise/develop' into state_cache

6ee61c8

Corey-Zumar added 7 commits January 21, 2018 15:24

Weak consistency - REVERT IF STRONG NECESSARY

2280e14

Revert "Weak consistency - REVERT IF STRONG NECESSARY"

6f17f91

This reverts commit 2280e14.

2-lock approach

b9c4441

Fix concurrency and consistency model

7b8644c

Docs

10281b3

Add docs

7424a06

Merge branch 'develop' into state_cache

bcb7d6d

Corey-Zumar added status: needs review and removed status: needs revision labels Jan 28, 2018

Merge branch 'develop' into state_cache

f0334ba

dcrankshaw approved these changes Feb 4, 2018

View reviewed changes

dcrankshaw added status: accepted and removed status: needs review labels Feb 4, 2018

Merge branch 'develop' into state_cache

4da739f

Merge branch 'develop' into state_cache

786a4c5

Corey-Zumar merged commit 137cc85 into ucbrise:develop Feb 7, 2018

Conversation

Corey-Zumar commented Jan 2, 2018

Uh oh!

AmplabJenkins commented Jan 2, 2018

Uh oh!

dcrankshaw commented Jan 3, 2018

Uh oh!

dcrankshaw left a comment

Choose a reason for hiding this comment

Uh oh!

dcrankshaw Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

Corey-Zumar Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

dcrankshaw Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

dcrankshaw Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

dcrankshaw Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jan 3, 2018

Uh oh!

Corey-Zumar commented Jan 21, 2018

Uh oh!

Corey-Zumar commented Jan 28, 2018

Uh oh!

AmplabJenkins commented Jan 28, 2018

Uh oh!

AmplabJenkins commented Jan 28, 2018

Uh oh!

Corey-Zumar commented Jan 28, 2018

Uh oh!

AmplabJenkins commented Jan 28, 2018

Uh oh!

dcrankshaw left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Feb 4, 2018

Uh oh!

AmplabJenkins commented Feb 5, 2018

Uh oh!

AmplabJenkins commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants