Create a MergeOperator for Cassandra Row Value #2289

scv119 · 2017-05-12T21:09:09Z

This PR implements the MergeOperator for Cassandra Row Values.

facebook-github-bot · 2017-05-12T21:22:31Z

@scv119 updated the pull request - view changes

facebook-github-bot · 2017-05-12T21:30:00Z

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

facebook-github-bot · 2017-05-12T22:03:06Z

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

facebook-github-bot · 2017-05-12T22:04:21Z

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

facebook-github-bot · 2017-05-13T00:18:11Z

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

facebook-github-bot · 2017-05-16T00:05:09Z

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

facebook-github-bot · 2017-05-16T00:27:54Z

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

adamretter

I can't comment too much on the C++ implementation itself because my C++ knowledge is not adequate. However, the Java and C++ JNI code looks fine to me. Nice job :-)

Just a few small comments mostly around cosmetics and whether you meant to implement the deprecated methods of rocksdb::MergeOperator rather than the newer ones?

adamretter · 2017-05-16T15:29:10Z

utilities/merge_operators/cassandra/format.cc

@@ -0,0 +1,291 @@
+/**


Should this be the standard RocksDB copyright header?

adamretter · 2017-05-16T15:30:17Z

utilities/merge_operators/cassandra/format.cc

+}
+
+std::size_t Column::Size() const {
+  return ColumnBase::Size() + 12 + value_size_;


Why 12? Is this a magic number? Would it make sense to define it as a static constant with a meaningful name?

adamretter · 2017-05-16T15:31:35Z

utilities/merge_operators/cassandra/format.cc

+  ttl_(ttl) {}
+
+std::size_t ExpiringColumn::Size() const {
+  return Column::Size() + 4;


Same question as previously. Why 4? Could this be a const of sizeof(type)?

adamretter · 2017-05-16T15:31:58Z

utilities/merge_operators/cassandra/format.cc

+}
+
+std::size_t Tombstone::Size() const {
+  return ColumnBase::Size() + 12;


12 - const/sizeof?

adamretter · 2017-05-16T15:32:20Z

utilities/merge_operators/cassandra/format.cc

+  columns_(std::move(columns)), last_modified_time_(last_modified_time) {}
+
+std::size_t RowValue::Size() const {
+  std::size_t size = 12;


12 - const/sizeof?

adamretter · 2017-05-16T15:39:47Z

utilities/merge_operators/cassandra/serialize_test.cc

@@ -0,0 +1,188 @@
+/**
+ * @author Chen Shen (chenshen@fb.com)


Should this be the standard RocksDB copyright header?

adamretter · 2017-05-16T15:39:52Z

utilities/merge_operators/cassandra/test_utils.cc

@@ -0,0 +1,63 @@
+/**
+ * Helper functions to create test row/column and verify their correctness.


Should this be the standard RocksDB copyright header?

adamretter · 2017-05-16T15:39:58Z

utilities/merge_operators/cassandra/test_utils.h

@@ -0,0 +1,41 @@
+/**
+ * Helper functions to create test row/column and verify their correctness.


Should this be the standard RocksDB copyright header?

adamretter · 2017-05-16T15:41:57Z

utilities/merge_operators/cassandra/serialize.h

+
+template<>
+inline void Serialize<int32_t>(int32_t t, std::string* dest) {
+  for (int i = 0; i < 4; i++) {


In this class there are many constant numbers used, such as 4, 8, 3 and 7. Could I guess that might be better as sizeof(int32_t), sizeof(int64_t), sizeof(int32_t) - 1 etc

adamretter · 2017-05-16T15:42:24Z

utilities/merge_operators/cassandra/format.cc

+std::size_t RowValue::Size() const {
+  std::size_t size = 12;
+  for (const auto& column : columns_) {
+    size += column -> Size();


Should be column->Size()

facebook-github-bot · 2017-05-16T18:29:05Z

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

facebook-github-bot · 2017-05-17T20:16:27Z

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

scv119 · 2017-05-19T07:37:59Z

thank you @adamretter I'll address the comments :)

facebook-github-bot · 2017-05-19T23:45:18Z

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

facebook-github-bot · 2017-05-22T18:38:37Z

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

facebook-github-bot · 2017-05-22T20:26:28Z

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

facebook-github-bot · 2017-06-06T19:21:43Z

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

facebook-github-bot · 2017-06-06T20:12:19Z

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

facebook-github-bot · 2017-06-06T20:33:22Z

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

facebook-github-bot · 2017-06-06T20:33:47Z

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

facebook-github-bot · 2017-06-06T20:42:28Z

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

facebook-github-bot · 2017-06-06T20:55:13Z

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

facebook-github-bot · 2017-06-06T21:46:07Z

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

facebook-github-bot · 2017-06-06T22:18:03Z

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

facebook-github-bot · 2017-06-06T22:45:30Z

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

sagar0 · 2017-06-14T00:07:03Z

Restarted a few travis tests which failed, to verify all the tests are passing before accepting.

sagar0

just a few nits. lgtm to go once these are fixed.
I'll run the make unity_test on my side to verify if there are any issues with filename collisions.

sagar0 · 2017-06-14T18:54:01Z

utilities/merge_operators/cassandra/format.h

+#include <memory>
+#include "rocksdb/merge_operator.h"
+#include "rocksdb/slice.h"
+#include "util/testharness.h"


Wondering if testharness is really needed in this file?

sagar0 · 2017-06-14T19:01:25Z

utilities/merge_operators/cassandra/merge_operator.cc

+#include <memory>
+#include <assert.h>
+
+#include "format.h"


Seems like this should be utilities/merge_operators/cassandra/format.h following the convention in other files.

sagar0 · 2017-06-14T19:02:53Z

utilities/merge_operators/cassandra/row_merge_test.cc

+
+#include <memory>
+#include "format.h"
+#include "test_utils.h"


Lets use Lets use "utilties/merge_operators/cassandra/" prefix

sagar0 · 2017-06-14T19:06:20Z

utilities/merge_operators/cassandra/cassandra_merge_test.cc

+#include "rocksdb/db.h"
+#include "rocksdb/merge_operator.h"
+#include "rocksdb/utilities/db_ttl.h"
+#include "test_utils.h"


Lets use "utilties/merge_operators/cassandra/" prefix here too.

sagar0 · 2017-06-14T19:23:41Z

utilities/merge_operators/cassandra/format_test.cc

+#include <cstring>
+#include <memory>
+#include "format.h"
+#include "serialize.h"


Lets use "utilties/merge_operators/cassandra/" prefix here too.

sagar0 · 2017-06-14T19:27:53Z

utilities/merge_operators/cassandra/merge_operator.h

+// of patent rights can be found in the PATENTS file in the same directory.
+
+#pragma once
+#include "rocksdb/merge_operator.h"


Wondering if file name collisions (merge_operator.h, format.h) will lead to unity_test failure?! Checking on my side by running make unity_test ...

Checked unity_test, and it looks good on scv119:merge-ops branch.

sagar0 · 2017-06-14T19:30:52Z

utilities/merge_operators/cassandra/merge_operator.h

+// Copyright (c) 2017-present, Facebook, Inc.  All rights reserved.
+// 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.


Add the couple of lines that were added in #2226, for GPLv2, for all the new files.

sagar0 · 2017-06-14T21:03:40Z

Made sure that all travis tests are passing. RateLimiterTest.Rate test on OSX is failing due to unrelated reasons. Fixing that in #2451 .

scv119 · 2017-06-16T18:30:20Z

utilities/merge_operators/cassandra/format.h

+  std::vector<std::unique_ptr<ColumnBase>> columns_;
+  int64_t last_modified_time_;
+
+  FRIEND_TEST(RowValueMergeTest, Merge);


@sagar0 FRIEND_TEST macro is from testharness header.

facebook-github-bot · 2017-06-16T18:56:22Z

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

facebook-github-bot · 2017-06-16T19:00:56Z

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

facebook-github-bot · 2017-06-16T19:37:20Z

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

sagar0 · 2017-06-16T21:02:43Z

Thanks for making the changes. Accepted, and Landing this now.

scv119 · 2017-06-16T21:38:17Z

👍 thank you for the review!

Summary: This PR implements the MergeOperator for Cassandra Row Values. Closes #2289 Differential Revision: D5055464 Pulled By: scv119 fbshipit-source-id: 45f276ef8cbc4704279202f6a20c64889bc1adef

facebook-github-bot added the CLA Signed label May 12, 2017

scv119 force-pushed the merge-ops branch from da540f5 to f8512cb Compare May 12, 2017 21:22

scv119 force-pushed the merge-ops branch from f8512cb to 436bd06 Compare May 12, 2017 22:03

scv119 force-pushed the merge-ops branch from 436bd06 to 9153201 Compare May 12, 2017 22:04

yiwu-arbug requested review from sagar0 and adamretter May 15, 2017 22:12

scv119 force-pushed the merge-ops branch from 9153201 to 58ee2be Compare May 16, 2017 00:05

scv119 force-pushed the merge-ops branch from 58ee2be to 94a8f23 Compare May 16, 2017 00:27

adamretter reviewed May 16, 2017

View reviewed changes

adamretter added the java-api label May 16, 2017

scv119 force-pushed the merge-ops branch from 94a8f23 to fecb296 Compare May 16, 2017 18:28

sagar0 mentioned this pull request May 16, 2017

Support MergeOperator in Java #2282

Open

scv119 force-pushed the merge-ops branch from fecb296 to 3333544 Compare May 17, 2017 20:16

scv119 force-pushed the merge-ops branch from 3333544 to 4976b74 Compare May 19, 2017 23:45

scv119 force-pushed the merge-ops branch from 4976b74 to a0a2b9a Compare May 22, 2017 18:38

scv119 force-pushed the merge-ops branch from a0a2b9a to d8fce41 Compare May 22, 2017 20:26

scv119 force-pushed the merge-ops branch from d8fce41 to 1a25df7 Compare May 22, 2017 23:06

scv119 force-pushed the merge-ops branch from a116c65 to 840aef6 Compare June 6, 2017 20:32

scv119 force-pushed the merge-ops branch from 840aef6 to 9ccc1c4 Compare June 6, 2017 20:40

scv119 force-pushed the merge-ops branch from 9ccc1c4 to e08acd1 Compare June 6, 2017 21:46

scv119 force-pushed the merge-ops branch from e08acd1 to 9dcd120 Compare June 6, 2017 22:17

sagar0 suggested changes Jun 14, 2017

View reviewed changes

scv119 commented Jun 16, 2017

View reviewed changes

merge ops

c8be647

scv119 force-pushed the merge-ops branch from 9dcd120 to f48bd94 Compare June 16, 2017 18:56

fix license and use absolute path for header

4be6dd0

scv119 force-pushed the merge-ops branch from f48bd94 to 4be6dd0 Compare June 16, 2017 19:00

sagar0 approved these changes Jun 16, 2017

View reviewed changes

facebook-github-bot closed this in cbd825d Jun 16, 2017

adamretter mentioned this pull request Dec 7, 2017

java UnsatisfiedLinkError when new CassandraValueMergeOperator() #3206

Closed

		@@ -0,0 +1,63 @@
		/**
		* Helper functions to create test row/column and verify their correctness.

		@@ -0,0 +1,41 @@
		/**
		* Helper functions to create test row/column and verify their correctness.

Create a MergeOperator for Cassandra Row Value #2289

Create a MergeOperator for Cassandra Row Value #2289

Conversation

scv119 commented May 12, 2017 • edited Loading

facebook-github-bot commented May 12, 2017

facebook-github-bot commented May 12, 2017

facebook-github-bot commented May 12, 2017

facebook-github-bot commented May 12, 2017

facebook-github-bot commented May 13, 2017

facebook-github-bot commented May 16, 2017

facebook-github-bot commented May 16, 2017

adamretter left a 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

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 May 16, 2017

facebook-github-bot commented May 17, 2017

scv119 commented May 19, 2017

facebook-github-bot commented May 19, 2017

facebook-github-bot commented May 22, 2017

facebook-github-bot commented May 22, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

sagar0 commented Jun 14, 2017 • edited Loading

sagar0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagar0 Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

sagar0 Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

sagar0 Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

sagar0 Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagar0 commented Jun 14, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Jun 16, 2017

facebook-github-bot commented Jun 16, 2017

facebook-github-bot commented Jun 16, 2017

sagar0 commented Jun 16, 2017

scv119 commented Jun 16, 2017

scv119 commented May 12, 2017 •

edited

Loading

sagar0 commented Jun 14, 2017 •

edited

Loading

sagar0 Jun 14, 2017 •

edited

Loading

sagar0 Jun 14, 2017 •

edited

Loading

sagar0 Jun 14, 2017 •

edited

Loading

sagar0 Jun 14, 2017 •

edited

Loading