Ash/Selva - Added full and prefix scan support to all the stores #61

ashwanthkumar · 2016-12-09T00:13:27Z

Changed ConcurrentHashMap of InMemoryStore to ConcurrentSkipListMap to preserve ordering of keys

Added tests to RocksDB and InMemory stores.

@gsriram7 Creating a PR out of the branch - so it's easy to track the work we do and review changes easily.

Changed ConcurrentHashMap of InMemoryStore to ConcurrentSkipListMap to preserve ordering of keys Added tests to RocksDB and InMemory stores

ashwanthkumar · 2016-12-09T00:15:23Z

suuchi-rocksdb/src/test/scala/in/ashwanthkumar/suuchi/store/rocksdb/RocksDbStoreSpec.scala

 import org.scalatest.{BeforeAndAfter, FlatSpec}

 class RocksDbStoreSpec extends FlatSpec with BeforeAndAfter {
-  val ROCKSDB_TEST_LOCATION = "/tmp/suuchi-rocks-test"
+
+  val ROCKSDB_TEST_LOCATION = "/tmp/suuchi-rocks-test/"


@gsriram7 Instead of using this hard-coded path see if we can use the Files.createTempDirectory. This would ensure we automatically choose a temp directory based on the platform.

ashwanthkumar · 2016-12-09T00:16:25Z

suuchi-rocksdb/src/test/scala/in/ashwanthkumar/suuchi/store/rocksdb/RocksDbStoreSpec.scala

  before {
    FileUtils.deleteDirectory(new File(ROCKSDB_TEST_LOCATION))
+    db = new RocksDbStore(RocksDbConfiguration.apply(ROCKSDB_TEST_LOCATION))


@gsriram7 I thought we reverted this change - do we really need to extract this out as a var db and use it? Else was it breaking the tests?

Yes it was breaking because of the LOCK.

ashwanthkumar · 2016-12-09T00:17:05Z

suuchi-rocksdb/src/main/scala/in/ashwanthkumar/suuchi/store/rocksdb/RocksDbStore.scala

+        key.length >= prefix.length && JArrays.equals(key.take(prefix.length), prefix)
+      }
+    }
+    iterator


@gsriram7 See if we can inline this variable.

ashwanthkumar · 2016-12-09T00:18:49Z

suuchi-core/src/main/scala/in/ashwanthkumar/suuchi/store/InMemoryStore.scala

+    store.entrySet().map(kv => KV(kv.getKey.array(), kv.getValue)).iterator
+  }
+
+  private def hasPrefix(key: Array[Byte], prefix: Array[Byte]) = {


@gsriram7 This looks identical to one on RocksDBStore. Can you see if you can extract this out as a reusable method inside an Object ?

codecov-io · 2016-12-09T00:24:43Z

Current coverage is 74.82% (diff: 81.48%)

No coverage report found for master at 4ce8583.

Powered by Codecov. Last update 4ce8583...c433ab7

Added specs to VersioneStore Extracted out ByteArrayUtils

ashwanthkumar · 2016-12-10T01:09:16Z

suuchi-core/src/test/scala/in/ashwanthkumar/suuchi/utils/ByteArrayUtilsSpec.scala

+
+class ByteArrayUtilsSpec extends FlatSpec {
+
+  "ByteArrayUtils" should "say whether a given byte array starts with a specified byte array prefix" in {


@gsriram7 Given we've length check it would be good to add a case where prefix is > in length than the key.

@ashwanthkumar I have that test already. The coverage was 100% for ByteArrayUtils.

ByteArrayUtils.hasPrefix("string".getBytes, prefix = "longerString".getBytes) should be(false)

@gsriram7 Sorry - I missed it, my bad. Good job!

@ashwanthkumar I guess the test description wasn't self explanatory. I ll put that into a new test n push..

ashwanthkumar · 2016-12-10T01:14:59Z

suuchi-core/src/main/scala/in/ashwanthkumar/suuchi/store/Store.scala

+    case _ => false
+  }
+
+  override def toString: String = new String(key) + " " + new String(value)


@gsriram7 If we're not using this - let's remove this method.

Yes. I just added it to debug

ashwanthkumar · 2016-12-11T05:00:48Z

@gsriram7 The changes LGTM. Let me know if you've anything more to add before we merge.

Added a utility to check whether hash of a given key is within specified range

ashwanthkumar · 2016-12-17T11:03:23Z

suuchi-core/src/main/scala/in/ashwanthkumar/suuchi/rpc/SuuchiScanService.scala

+      .build()
+  }
+
+  private def buildResponse(response: KV): ScanResponse = {


Please prefer to push all private def to bottom of the class.

ashwanthkumar · 2016-12-17T11:04:45Z

suuchi-core/src/test/scala/in/ashwanthkumar/suuchi/rpc/SuuchiScanServiceTest.scala

+
+  private def populateStore(num: Int, store: Store) = 1 to num foreach (i => store.put(i.toString.getBytes, (i * 100).toString.getBytes))
+
+  private def extractKey(response: ScanResponse) = new String(response.getKv.getKey.toByteArray).toInt


As above - please push the private methods to the bottom below the tests.

ashwanthkumar · 2016-12-17T11:05:08Z

suuchi-core/src/test/scala/in/ashwanthkumar/suuchi/rpc/SuuchiScanServiceTest.scala

+  "SuuchiScanService" should "support scan for a given token range" in {
+
+    val store = new InMemoryStore
+    lazy val service = new SuuchiScanService(store)


Is there a reason for marking the service as lazy here?

ashwanthkumar · 2016-12-17T11:06:18Z

suuchi-core/src/main/proto/suuchi.proto

+    rpc Scan (ScanRequest) returns (stream ScanResponse);
+}
+message ScanRequest {
+    int32 start = 1;


It would really help to extract the start and end as TokenRange. Given we had plans to use the same thing with other store methods too.

TokenRange has nodes to which keys within that range will be routed. This info isn't needed for scans so just going with start and end as of now.

ashwanthkumar · 2016-12-17T11:12:15Z

suuchi-core/src/test/scala/in/ashwanthkumar/suuchi/utils/ByteArrayUtilsSpec.scala

+    val hashFn = mock(classOf[Hash])
+    val key = "1".getBytes
+
+    when(hashFn.hash(key)).thenReturn(1).thenReturn(100)


Okay I was confused with this. Can we please not use multiple thenReturn? Instead explicitly do

when(hashFn.hash(key)).thenReturn(1) ByteArrayUtils.isHashKeyWithinRange(10, 100, key, hashFn) should be(false) when(hashFn.hash(key)).thenReturn(100) ByteArrayUtils.isHashKeyWithinRange(1, 10, key, hashFn) should be(false)

It helps a lot in readability with just 1 extra line no? What do you think?

Yes it is explicit, I ll make the change..

ashwanthkumar · 2016-12-17T11:13:08Z

suuchi-core/src/test/scala/in/ashwanthkumar/suuchi/utils/ByteArrayUtilsSpec.scala

+    val key = "1".getBytes
+
+    when(hashFn.hash(key)).thenReturn(10)
+


Nit picking - Please remove this empty line.

Intentionally left it. For projects like zeus we follow convention like

Variable inits \n

Mock interactions\n

Actual action and assertion

Didn't like it?

ashwanthkumar · 2016-12-17T11:16:47Z

suuchi-core/src/main/scala/in/ashwanthkumar/suuchi/store/VersionedStore.scala

@@ -107,4 +111,8 @@ class VersionedStore(store: Store, versionedBy: VersionedBy, numVersions: Int, c

  private def removeData(key: Array[Byte], version: Long) = store.remove(dkey(key, version))
  private def removeVersion(key: Array[Byte]) = store.remove(vkey(key))
+


Selva - we talked about adding a new method scanVersions (with 2 overloaded variants like scan) that does prefix based on VersionedStore.vkey in VersionedStore. I find that missing. Can you please add that too?

We don't have an immediate need for it but that goes with symmetry for VersionedStore given we can scan both just for versions and for records.

Yes then we agreed that scan should blindly give away all the versions right?

Ohh it's the version store. Yes, I ll add an overloaded method.

ashwanthkumar · 2016-12-19T11:15:33Z

@gsriram7 This LGTM. I'm going ahead with the merge. Please let's take further PR comments as separate PRs.

ashwanthkumar · 2016-12-19T11:12:29Z

suuchi-core/src/main/scala/in/ashwanthkumar/suuchi/store/VersionedStore.scala

+
+  def toVRecord(kv: KV) = VRecord(kv.key, Versions.fromBytes(kv.value))
+
+  def scanVersions(prefix: Array[Byte]): Iterator[VRecord] = store.scan(vkey(prefix)).map(toVRecord)


Please also add the scanVersions method without the prefix as well.

Ash/Selva - Added full and prefix scan support to all the stores

075fdfb

Changed ConcurrentHashMap of InMemoryStore to ConcurrentSkipListMap to preserve ordering of keys Added tests to RocksDB and InMemory stores

ashwanthkumar commented Dec 9, 2016

View reviewed changes

Ash/Selva - Incorporating PR reviews

342be3b

Added specs to VersioneStore Extracted out ByteArrayUtils

ashwanthkumar commented Dec 10, 2016

View reviewed changes

gsriram7 added 3 commits December 13, 2016 13:09

Incorporating PR reviews

66cffc6

Added Suuchi scan streaming service

232fed9

Added a utility to check whether hash of a given key is within specified range

Changed a variable name

afb3cc0

ashwanthkumar commented Dec 17, 2016

View reviewed changes

ashwanthkumar and others added 4 commits December 18, 2016 14:20

Merge branch 'master' into suuchi-scan

728d70c

[suuchi-core] Moving private method to bottom in CHRing

82a53f5

Incorporating PR reviews

99e6451

Added version scan for VersionedStore

c433ab7

ashwanthkumar commented Dec 19, 2016

View reviewed changes

ashwanthkumar merged commit 85622ca into master Dec 19, 2016

ashwanthkumar deleted the suuchi-scan branch December 19, 2016 11:15

ashwanthkumar added this to the 0.2 milestone Jun 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ash/Selva - Added full and prefix scan support to all the stores #61

Ash/Selva - Added full and prefix scan support to all the stores #61

ashwanthkumar commented Dec 9, 2016

ashwanthkumar Dec 9, 2016 •

edited

Loading

ashwanthkumar Dec 9, 2016

ashwanthkumar Dec 9, 2016

ashwanthkumar Dec 9, 2016

ashwanthkumar Dec 9, 2016

codecov-io commented Dec 9, 2016 •

edited

Loading

ashwanthkumar Dec 10, 2016

gsriram7 Dec 10, 2016 •

edited

Loading

ashwanthkumar Dec 11, 2016

gsriram7 Dec 11, 2016

ashwanthkumar Dec 10, 2016

gsriram7 Dec 10, 2016

ashwanthkumar commented Dec 11, 2016

ashwanthkumar Dec 17, 2016

ashwanthkumar Dec 17, 2016

ashwanthkumar Dec 17, 2016

ashwanthkumar Dec 17, 2016

gsriram7 Dec 19, 2016 •

edited

Loading

ashwanthkumar Dec 17, 2016

gsriram7 Dec 17, 2016

ashwanthkumar Dec 17, 2016

gsriram7 Dec 19, 2016

ashwanthkumar Dec 17, 2016

gsriram7 Dec 17, 2016

gsriram7 Dec 17, 2016

ashwanthkumar commented Dec 19, 2016

ashwanthkumar Dec 19, 2016


		class ByteArrayUtilsSpec extends FlatSpec {

		"ByteArrayUtils" should "say whether a given byte array starts with a specified byte array prefix" in {


		private def populateStore(num: Int, store: Store) = 1 to num foreach (i => store.put(i.toString.getBytes, (i * 100).toString.getBytes))

		private def extractKey(response: ScanResponse) = new String(response.getKv.getKey.toByteArray).toInt

		@@ -107,4 +111,8 @@ class VersionedStore(store: Store, versionedBy: VersionedBy, numVersions: Int, c

		private def removeData(key: Array[Byte], version: Long) = store.remove(dkey(key, version))
		private def removeVersion(key: Array[Byte]) = store.remove(vkey(key))


		def toVRecord(kv: KV) = VRecord(kv.key, Versions.fromBytes(kv.value))

		def scanVersions(prefix: Array[Byte]): Iterator[VRecord] = store.scan(vkey(prefix)).map(toVRecord)

Ash/Selva - Added full and prefix scan support to all the stores #61

Ash/Selva - Added full and prefix scan support to all the stores #61

Conversation

ashwanthkumar commented Dec 9, 2016

ashwanthkumar Dec 9, 2016 • edited Loading

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

codecov-io commented Dec 9, 2016 • edited Loading

Current coverage is 74.82% (diff: 81.48%)

Choose a reason for hiding this comment

gsriram7 Dec 10, 2016 • edited Loading

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

ashwanthkumar commented Dec 11, 2016

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

gsriram7 Dec 19, 2016 • edited Loading

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

ashwanthkumar commented Dec 19, 2016

Choose a reason for hiding this comment

ashwanthkumar Dec 9, 2016 •

edited

Loading

codecov-io commented Dec 9, 2016 •

edited

Loading

gsriram7 Dec 10, 2016 •

edited

Loading

gsriram7 Dec 19, 2016 •

edited

Loading