[rocksandra] Cassandra compaction filter for purge expired columns and rows #2588

wpc · 2017-07-15T00:14:40Z

Major changes in this PR:

Implement CassandraCompactionFilter to remove expired columns and rows (if all column expired)
Move cassandra related code from utilities/merge_operators/cassandra to utilities/cassandra/*
Switch to use shared_ptr<> from uniqu_ptr for Column membership management in RowValue. Since columns do have multiple owners in Merge and GC process, use shared_ptr helps make RowValue immutable.
Rename cassandra_merge_test to cassandra_functional_test and add two TTL compaction related tests there.

facebook-github-bot · 2017-07-15T00:14:47Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2017-07-15T00:20:18Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2017-07-15T00:21:23Z

@wpc updated the pull request - view changes

facebook-github-bot · 2017-07-15T00:37:55Z

@wpc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-07-15T02:50:32Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-15T02:51:48Z

@wpc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-07-15T03:43:48Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-15T03:45:24Z

@wpc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-07-15T05:59:12Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-15T23:52:32Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-15T23:54:57Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-16T00:42:23Z

@wpc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-07-16T05:17:57Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-16T06:49:30Z

@wpc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

scv119 · 2017-07-17T04:30:19Z

utilities/cassandra/format.h

@@ -82,8 +82,8 @@ class ColumnBase {
  virtual int8_t Index() const;
  virtual std::size_t Size() const;
  virtual void Serialize(std::string* dest) const;
-
-  static std::unique_ptr<ColumnBase> Deserialize(const char* src,
+  virtual bool Collectable() const = 0;


GarbageCollectable for clarification maybe?

scv119 · 2017-07-17T04:38:49Z

utilities/cassandra/format.cc

@@ -112,7 +116,11 @@ void ExpiringColumn::Serialize(std::string* dest) const {
  rocksdb::cassandra::Serialize<int32_t>(ttl_, dest);
 }

-std::unique_ptr<ExpiringColumn> ExpiringColumn::Deserialize(
+bool ExpiringColumn::Collectable() const {
+  return Timestamp() + ttl_ * 1000000 < time(NULL) * 1000000;


not sure if we should use c++ 11 time style here: http://en.cppreference.com/w/cpp/chrono/time_point

I believe we should turn it into a tombstone first, then compaction filter kicks in according to gc_grace_period.

scv119 · 2017-07-17T04:39:57Z

utilities/cassandra/format.cc

+  // * and there are no live data for the key in higher level.
+  // We cannot get info for the second condidtion yet, so we
+  // will just keep tombstone around for now.
+  return false;


one possible solution is that we could make the gc_grace_seconds as part of compaction filter's constructor.

Yeah. But we still do not have information on whether there are live data on the higher level sstable files yet. It feels weird passing in gc_grace_seconds but has no place using it.

facebook-github-bot · 2017-07-19T04:58:15Z

@wpc updated the pull request - view changes - changes since last import

wpc · 2017-07-19T05:04:08Z

@scv119 I updated the PR:

use std::chrono lib for current time in microseconds
make the behavior purging or converting to tombstone configurable. And in the java code will set default behavior to converting expired column to tombstone

facebook-github-bot · 2017-07-19T08:18:21Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-19T09:03:54Z

@wpc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-07-19T19:45:12Z

@wpc updated the pull request - view changes - changes since last import

scv119 · 2017-07-19T19:54:37Z

java/rocksjni/cassandra_compactionfilterjni.cc

+// This source code is licensed under the BSD-style license found in the
+// LICENSE file in the root directory of this source tree. An additional grant
+// of patent rights can be found in the PATENTS file in the same directory.
+


// This source code is also licensed under the GPLv2 license found in the // COPYING file in the root directory of this source tree.

I believe we also need add this two lines to all source code.

We recently updated the license to Apache+GPLv2, and updated the copyright headers in all the files in RocksDB. So please use those headers here and in all the new files.

DikangGu · 2017-07-19T20:45:33Z

utilities/cassandra/format.cc

+  }
+  return RowValue(std::move(new_columns), last_modified_time_);
+
+  return RowValue(columns_, last_modified_time_);


duplicated returns

scv119 · 2017-07-19T20:12:41Z

utilities/cassandra/cassandra_compaction_filter.h

+namespace rocksdb {
+namespace cassandra {
+
+class CassandraCompactionFilter : public CompactionFilter {


Might worth add some comments on what's the expected behavior of this filter, and more importantly, the option purge_ttl_on_expiration

scv119 · 2017-07-19T20:15:53Z

utilities/cassandra/format.h

@@ -159,6 +166,9 @@ class RowValue {
  // otherwise it returns the max timestamp of containing columns.
  int64_t LastModifiedTime() const;
  void Serialize(std::string* dest) const;
+  RowValue PurgeTTL(bool* changed) const;


Ttl or TTL ? not sure the convention in Rocksdb but I see both TTL and Ttl is used in this PR.

scv119 · 2017-07-19T20:20:28Z

utilities/cassandra/format.cc

+  }
+  return RowValue(std::move(new_columns), last_modified_time_);
+
+  return RowValue(columns_, last_modified_time_);


remove this line?

scv119 · 2017-07-19T20:27:56Z

utilities/cassandra/format.cc

+  for (auto& column : columns_) {
+    if(column->Expired()) {
+      if(shared_ptr<Tombstone> tombstone = column->ToTombstone()) {
+        new_columns.push_back(tombstone);


nit: use move instead of copy here should give us some performance gain.

scv119 · 2017-07-19T20:42:14Z

utilities/cassandra/format.cc

+}
+
+bool ExpiringColumn::Expired() const {
+  return Timestamp() + ttl_ * 1000000 < NowMicros();


http://en.cppreference.com/w/cpp/chrono/duration there are a bunch of utilities could help the conversion.

facebook-github-bot · 2017-07-19T22:20:36Z

@wpc updated the pull request - view changes - changes since last import

scv119

Logics looks good to me, few nits:

GPLv2 licence in the comment header?
consider using std::chrono:: to do the cast between second and microsecond.

I'll leave this to @sagar0 to approve.

facebook-github-bot · 2017-07-19T22:36:26Z

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

sagar0 · 2017-07-20T06:14:30Z

Thanks @wpc for contributing!

cassandra_functional_test.cc appears as a new file instead of as a "moved from cassandra_merge_test.cc". Not that it matters a whole lot since there was only 1 commit (of merge operator) prior to this PR, but we will lose that history. Do you think its possible to correct it?
Could you run 'make format' once so that some aspects of coding-style are taken care of.
Lets fix the copyright/license headers in cassandra_functional_test.cc, cassandra_compaction_filter.cc, CassandraCompactionFilter.java and cassandra_compactionfilterjni.cc.

sagar0 · 2017-07-20T17:41:40Z

utilities/cassandra/cassandra_format_test.cc

@@ -301,6 +302,51 @@ TEST(RowValueTest, RowWithColumns) {
    std::memcmp(dest.c_str(), dest.c_str() + r.Size(), r.Size()) == 0);
 }

+TEST(RowValueTest, PurgeTTLShouldRemvoeAllColumnExpired) {


s/RemvoeAllColumn/RemoveAllColumns/

sagar0 · 2017-07-20T17:42:02Z

utilities/cassandra/cassandra_format_test.cc

+  EXPECT_FALSE(changed);
+}
+
+TEST(RowValueTest, ExpiresTTLShouldConvertExpiredColumnsToTombstone) {


s/Expires/Expired/

facebook-github-bot · 2017-07-21T00:40:04Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-21T01:00:28Z

@wpc updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-21T01:33:57Z

@wpc updated the pull request - view changes - changes since last import

wpc · 2017-07-21T01:50:15Z

@sagar0 and @scv119 I updated the PR base on the review feedback:

use correct license file header (https://fburl.com/v2otiqbi)
use std:chrono for time point conversion and comparison (https://fburl.com/v5quy8ip) (this actually fixed an int overflow bug)
some typo fixes (https://fburl.com/37iiw3l0)
Use "*Ttl" instead of "*TTL" for consistency
add document to CassandraCompactionFilter (https://fburl.com/xdfakph4)

facebook-github-bot · 2017-07-21T02:35:15Z

@wpc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed label Jul 15, 2017

wpc force-pushed the master branch from 1573504 to 46e28bc Compare July 15, 2017 00:21

wpc force-pushed the master branch from 46e28bc to da5addf Compare July 15, 2017 02:50

wpc force-pushed the master branch from da5addf to 7b03b4b Compare July 15, 2017 03:43

wpc force-pushed the master branch from 7b03b4b to e1b7221 Compare July 15, 2017 05:58

wpc force-pushed the master branch from e1b7221 to e4b0f20 Compare July 15, 2017 23:52

wpc force-pushed the master branch from e4b0f20 to b5df568 Compare July 15, 2017 23:54

wpc force-pushed the master branch from b5df568 to 9415060 Compare July 16, 2017 05:17

scv119 reviewed Jul 17, 2017

View reviewed changes

wpc force-pushed the master branch from 9415060 to b4c24a7 Compare July 19, 2017 04:58

wpc force-pushed the master branch from b4c24a7 to 6868cc6 Compare July 19, 2017 08:18

wpc force-pushed the master branch from 6868cc6 to 8cc52e7 Compare July 19, 2017 19:45

scv119 reviewed Jul 19, 2017

View reviewed changes

DikangGu reviewed Jul 19, 2017

View reviewed changes

scv119 reviewed Jul 19, 2017

View reviewed changes

wpc force-pushed the master branch from 8cc52e7 to 0fb3b08 Compare July 19, 2017 22:20

scv119 reviewed Jul 19, 2017

View reviewed changes

sagar0 reviewed Jul 20, 2017

View reviewed changes

wpc force-pushed the master branch from 0fb3b08 to 37094a4 Compare July 21, 2017 00:39

wpc force-pushed the master branch from 37094a4 to a140ee1 Compare July 21, 2017 01:00

cassandra compaction filter for purge expired ttl columns and rows

d3d1d5d

wpc force-pushed the master branch from a140ee1 to d3d1d5d Compare July 21, 2017 01:33

sagar0 approved these changes Jul 21, 2017

View reviewed changes

facebook-github-bot closed this in 534c255 Jul 21, 2017

[rocksandra] Cassandra compaction filter for purge expired columns and rows #2588

[rocksandra] Cassandra compaction filter for purge expired columns and rows #2588

Conversation

wpc commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 15, 2017

facebook-github-bot commented Jul 16, 2017

facebook-github-bot commented Jul 16, 2017

facebook-github-bot commented Jul 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 19, 2017

wpc commented Jul 19, 2017

facebook-github-bot commented Jul 19, 2017

facebook-github-bot commented Jul 19, 2017

facebook-github-bot commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 19, 2017

scv119 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 19, 2017

sagar0 commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 21, 2017

facebook-github-bot commented Jul 21, 2017

facebook-github-bot commented Jul 21, 2017

wpc commented Jul 21, 2017

facebook-github-bot commented Jul 21, 2017