Geode 2469 #404

ggreen · 2017-02-19T04:22:13Z

The updated Geode Redis Adapter now works with a sample Spring Data Redis Example
GEODE-2469.pdf

These changes are focused on the HASH and Set Redis Data Types to support Spring Data Redis sample code located at the following URL

https://github.com/Pivotal-Data-Engineering/gemfire9_examples/tree/person_example_sdg_Tracker139498217/redis/spring-data-redis-example/src/test/java/io/pivotal/redis/gemfire/example/repository

The Hash performance changes from this pull request had a 99.8% performance improvement.

This is based on the Hashes JUNIT Tests.

https://github.com/Pivotal-Data-Engineering/gemfire9_examples/blob/person_example_sdg_Tracker139498217/redis/gemfire-streaming-client/src/test/java/io/pivotal/gemfire9/HashesJUnitTest.java

This code executed in 12.549s against Gemfire 9.0.1 code. After the changes, the test executed in 0.022s with the GEODE-2469 pull request.

Redis Set related command performance had a 99.9% performance improvement.

See https://github.com/Pivotal-Data-Engineering/gemfire9_examples/blob/person_example_sdg_Tracker139498217/redis/gemfire-streaming-client/src/test/java/io/pivotal/gemfire9/SetsJUnitTest.java

The previous Set Junit tests executed against GemFire 9.0.1 executed in 31.507 seconds. These same test executed in 0.036 seconds with the GEODE-2469 pull request changes.

The GemFire 9.0.1 Geode (1.1.0) version for the Geode Redis adapter created a Geode Region for each key provided in the Redis Hash or Set related command. Each Redis command to remove key entry previously destroyed the region. The big performance gain is based on using a new ReDiS_HASH and ReDiS_SET region. Note the changed will create or reuse an existing region with a matching name for Redis HASH commands references objects. For Redis HASH object's key will have a format of object:key

Please see https://redis.io/topics/data-types-intro HASH section on objects for information on Redis objects.

bschuchardt

Hi ggreen, would you update the PR to include the missing SetInterpreter class?

ggreen · 2017-02-22T17:59:41Z

Hello, Sorry about that. It is there now geode/SetExecutor.java at GEODE-2469 · ggreen/geode · GitHub | | | | | | | | | | | geode/SetExecutor.java at GEODE-2469 · ggreen/geode · GitHub geode - Mirror of Apache Geode | | | | On Wednesday, February 22, 2017 12:21 PM, Bruce Schuchardt <notifications@github.com> wrote: @bschuchardt requested changes on this pull request.Hi ggreen, would you update the PR to include the missing SetInterpreter class?— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

metatype · 2017-02-25T00:40:53Z

Thanks for the contribution. Could you fix the test failure in RegionProviderTest?

kohlmu-pivotal · 2017-02-27T15:10:46Z

I like the fact that we are now dealing with collections on a single machine, rather than all the elements spread distributed. Would you possibly have some perf metrics in relation to larger collections. e.g 1mil entries vs 100.

I imagine that as the number of entries in the collections grow, so will the insert performance.

hiteshk25 · 2017-02-28T16:38:21Z

geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java

+          ByteArrayWrapper regionName = HashInterpreter.toRegionNameByteArray(key);
+          Region<?, ?> region = this.getRegion(regionName);
+          if (region != null) {
+            region.remove(HashInterpreter.toEntryKey(key));


Needs to return from here?

We could return true, but that is what the next line after the if statement already returns true
.

hiteshk25

just to track following issues

I think the intention was to create a region for Hashes like "huser:1000", but it is putting key "1000" in the region "region=/ReDiS_HASH". Look following logs

oRegionNameByteArray keyString huser:1000
hashinterpreter getRegion huser
[debug 2017/03/01 14:11:19.675 PST tid=28] PR.virtualPut putting event=EntryEventImpl[op=UPDATE;region=/ReDiS_HASH;key=1000;oldValue=null;newValue={};callbackArg=null;originRemote=false;originMember=hkhamesra-T440p(11928:loner):0:1870ed8b;id=EventID[threadID=2;sequenceID=3]]

HIncrByFloatExecutor: we are incrementing field value but not saving that back into region
HExistsExecutor: We are not looking value inside map
Need to look metaType region logic with new changes; (Fixed this in HExistsExecutor)

hiteshk25 · 2017-02-28T16:48:05Z

geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java

+
+    String regionName = key.toString();
+
+    // check if type is hash TODO: remove


do we need to look hash key here?

I think we can safely remove this TODO comment.
The special processing for hashing is now handled at the HashExecutor (along with its sub-classes) commands level. It does not need to be handled at the RegionProvider level.

Hello @galen-pivotal ,

You mean why do we need to create a region for an object, correct?
If you mean that, then yes, I guess we could just use the same ReDiS_HASH region for objects and non-objects. Of course, there are pros and cons for each approach.

Prior to this pull request, the Redis adapter created objects for all HASH keys. I originally liked the idea of limiting the creation of regions to just HASH objects. Having a separate region when objects are used can provide additional flexibility. For example, we could pre-define the region with unique definitions based on a given use case.

While testing with Spring Data Redis example, I created a "companies" region with my desired properties that were totally under my control. These regions for objects could be managed separately from the default ReDiS_HASH region. For example, we could different data policies, disk stores, listeners, etc.

What do you think?

ggreen · 2017-03-02T18:59:01Z

RE: "huser:1000

The intent is to detect the use of objects in the Redis HASH commands. The character ":" will appear in the key if an object is used (note: this is unique for HASH commands). The format of the key will be object:key.

So if a HASH command was used with the key "huser:1000", this should have resulted in an "huser" region being created @with a key of "1000". If this did not happen, this is a bug.

The /ReDiS_HASH region should be used in cases where ":" does not exist in the key (because this is not necessarily an object).

I can fix this along with the indicated issues (2-4)

galen-pivotal · 2017-03-02T19:42:00Z

@ggreen
Is there any reason we need to create objects rather than just storing under the given string?

galen-pivotal

@ggreen Redis users often use ":" to number users or other objects (the Redis docs mention this use), but Redis itself isn't aware of the "types" of data -- it just stores them as hashes that can be looked up by strings. Putting the key in a hash region may semantically correspond with the usage of "user:1000" as being an object of type user, but according to the API it's just keys to hashes. I think that having two different routes based on the type of key is unneeded complexity and extra code. I think we should do what's fastest, and add an abstraction later if we want to access data with a particular lens.

The idea of having something in Java that matches the common use case of using hashes to store objects in Redis is nice, though. If we stabilize the API on something that is fast enough, it could be a useful abstraction to access what's already stored.

galen-pivotal · 2017-03-02T21:17:22Z

geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java

@@ -442,9 +502,10 @@ protected void checkDataType(ByteArrayWrapper key, RedisDataType type) {
      return;
    if (currentType == RedisDataType.REDIS_PROTECTED)
      throw new RedisDataTypeMismatchException("The key name \"" + key + "\" is protected");
-    if (currentType != type)


Why has this been removed?

galen-pivotal · 2017-03-02T22:02:07Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HDelExecutor.java

+    // save map
+    saveMap(map, context, key);
+
+    // if (map.isEmpty()) {


Would prefer to remove code rather than comment it out.

galen-pivotal · 2017-03-02T22:02:57Z

geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java

-    if (currentType != type)
-      throw new RedisDataTypeMismatchException(
-          "The key name \"" + key + "\" is already used by a " + currentType.toString());
+    /*


Would prefer to remove code rather than comment it out.

galen-pivotal · 2017-03-02T23:49:50Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HashInterpreter.java

+   * @return the region were the command's data will be processed
+   */
+  @SuppressWarnings("unchecked")
+  public static Region<ByteArrayWrapper, Map<ByteArrayWrapper, ByteArrayWrapper>> getRegion(


I think the abstraction would be cleaner if the HashInterpreter implemented setHash and getHash (and setHashKey and getHashKey) methods, and callers didn't have to think about regions at all. It would make it clearer where the single point of concern is for where and how hashes get stored.

@galen-pivotal I will look to implement your recommendations. One clarification question, You are recommending that we do not create a separate region for hash objects, but store everything in /ReDiS_HASH with the region key of object:key, correct?

@ggreen yeah, I'd put everything in one region because I think it's easier to understand, and because it's much cheaper to create new hash objects in a region than it is to create new regions. Though if you want to see my hidden agenda, look ahead:

if I were to redesign data storage in the Redis adapter, I think I would do away with the separate region per type and a metadata region that just stores types, and implement the whole thing as one region that stored collections of all the types we support. During the lookup we could catch a ClassCastException if the key is the wrong type, and then we'd propagate that up as the same error Redis throws when you try to modify a key that is of the wrong type.

Storing collections as Java objects rather than via some translation to a Region means that as the objects get larger, the cost of transferring them between members in the system increases as well. Geode contains a Delta interface that I think we could use to avoid the overhead of transferring the whole object every time. Then the only downside is that we can't scale a single hash/set/list across servers, which I think is fine -- do you really need to store a list in Redis that takes more than however many gigabytes of RAM are in a single Geode instance? Some folks on the user/dev lists seem to disagree with this view, though, so take it with a bit of salt.

The other thing I'd like to look at to implement this is how we want to store and process data -- the whole ByteArrayWrapper/String thing could be simplified, but I think that's fairly orthogonal to the region and command interpreter implementation.

I don't know that much about the end users of Redis, though, so I'm curious: what are you using the Redis adapter for? What use case are you optimizing for?

@galen-pivotal: I really like the idea of a single region. The less the number of regions, then the less overhead, but for now I just limited the change to using the Redis_Hash region. This same region will be used for HASH data types of objects and non-objects.
This pull request was originally based on an attempt to get some Spring Data Redis example code to work with the GemFire Redis adaptor.

Also, @wwilliams-pivotal and @hiteshk25: I believe I incorporated your observations.
I also tried to improve the integration tests for the hash related commands.

Please review my latest commit, and let me know if any additional changes are needed as a pre-requisite for accepting this pull request.

…n tests and enhancing documentation

galen-pivotal

Looks good! If we get a yes vote on marking the Redis adapter as experimental, I'm all for merging this pending a few changes. I put in a few comments, mostly minor. I'll do a bit of looking into the right way to lock entries.

galen-pivotal · 2017-03-06T23:32:33Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HashExecutor.java

+   * @param key the raw key
+   * @return the ByteArray for the key
+   */
+  public static ByteArrayWrapper toEntryKey(ByteArrayWrapper key) {


Can this be private?

galen-pivotal · 2017-03-08T20:00:01Z

geode-core/src/main/java/org/apache/geode/redis/internal/ExecutionHandlerContext.java

+      Command command = (Command) msg;
+      executeCommand(ctx, command);
+    } catch (Exception e) {
+      logger.error(e);


If we're going to be logging exceptions as they propagate, I'd like to think about the best place to catch all of them and also it would be nice to add a little error message so we know where we caught the exception.

galen-pivotal · 2017-03-08T20:00:15Z

geode-core/src/main/java/org/apache/geode/redis/internal/ExecutionHandlerContext.java

@@ -235,6 +242,8 @@ private void executeWithoutTransaction(final Executor exec, Command command) thr
        exec.executeCommand(command, this);
        return;
      } catch (Exception e) {
+        logger.error(e);


see error logging comment above

galen-pivotal · 2017-03-08T20:05:03Z

geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java

+      Region<ByteArrayWrapper, Set<ByteArrayWrapper>> setRegion) {
+
+    this(stringsRegion, hLLRegion, redisMetaRegion, expirationsMap, expirationExecutor,
+        defaultShortcut, hashRegion, setRegion, GemFireCacheImpl.getInstance());


👍 for making the cache an argument to the constructor (even if it's optional). Anything that makes things a little more decoupled.

galen-pivotal · 2017-03-08T20:07:55Z

geode-core/src/main/java/org/apache/geode/redis/internal/RegionProvider.java

+        } else if (type == RedisDataType.REDIS_LIST) {
+          return this.destroyRegion(key, type);
+        } else if (type == RedisDataType.REDIS_SET) {
+          // remove the set


I think the comment is redundant and the returns aren't necessary (& distract from overall control flow).

A comment saying "sets & hashes get removed from the region they're put in rather than destroying a region" would be fine.

galen-pivotal · 2017-03-08T23:08:08Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SIsMemberExecutor.java

+      command.setResponse(Coder.getIntegerResponse(context.getByteBufAllocator(), NOT_EXISTS));
+      return;
+    }
+


We should check the data type here, as Redis gives a different error for a key that is of the wrong datatype than it does for a nonexistent key.

galen-pivotal · 2017-03-08T23:34:16Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SMoveExecutor.java

      command.setResponse(Coder.getIntegerResponse(context.getByteBufAllocator(), NOT_MOVED));
      return;
    }

-    Object oldVal = sourceRegion.get(mem);
-    sourceRegion.remove(mem);
+    sourceSet = new HashSet<ByteArrayWrapper>(sourceSet); // copy to support transactions;


Copying helps with concurrent modifications on the same machine, but we can still lose set adds 'cause race conditions (A reads, B reads, A adds key "foo" to a copy, B adds key "bar" to a copy, A puts, B puts, the key added by A is lost). ~~I think the ExecutionHandlerContext provides transaction support...~~ txn support in ExecutionHandlerContext looks like it's only for MULTI/EXEC.

I think we can do transactions in Geode by locking a particular key, or two keys if they're on the same server. However, if they aren't then we'll just have to say "whoops!" and return an error (Redis cluster does the same thing).

Maybe just mark it as a FIXME for now, and carry on?

galen-pivotal · 2017-03-09T00:17:19Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SPopExecutor.java

-        (Region<ByteArrayWrapper, Boolean>) context.getRegionProvider().getRegion(key);
-    if (keyRegion == null || keyRegion.isEmpty()) {
+    // getRegion(key);
+    Region<ByteArrayWrapper, Set<ByteArrayWrapper>> region = getRegion(context);


We still need to check the metadata region to determine whether to throw WRONGTYPE or return nil.

galen-pivotal · 2017-03-09T00:24:30Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SRandMemberExecutor.java

-    @SuppressWarnings("unchecked")
-    Region<ByteArrayWrapper, Boolean> keyRegion =
-        (Region<ByteArrayWrapper, Boolean>) context.getRegionProvider().getRegion(key);
+


I think we still need to check the metadata region here again.

Perhaps we should make getRegion check the Metadata region and throw a checked TypeException if the type is wrong?

galen-pivotal · 2017-03-09T00:33:08Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/set/SetOpExecutor.java

-      firstSet = new HashSet<ByteArrayWrapper>(region.keySet());
-    }
-    ArrayList<Set<ByteArrayWrapper>> setList = new ArrayList<Set<ByteArrayWrapper>>();
+    // if (!isStorage())


This is gnarly. I'll have to look at this later.

galen-pivotal · 2017-03-10T23:31:48Z

geode-core/src/main/java/org/apache/geode/redis/internal/executor/hash/HSetExecutor.java

@@ -55,9 +69,11 @@ public void executeCommand(Command command, ExecutionHandlerContext context) {
    Object oldValue;

    if (onlySetOnAbsent())
-      oldValue = keyRegion.putIfAbsent(field, new ByteArrayWrapper(value));
+      oldValue = map.putIfAbsent(field, new ByteArrayWrapper(value));


This will fail with ConcurrentModificationException if other connections try to access the same hash at the same time. I think we need to clone, modify the clone and then put, which will give us eventual consistency but not ACID. (or lock the region, or a single key in the region if that's possible)

Use DLockService to avoid concurrency errors.

…DE-2469

metatype · 2017-03-20T16:46:29Z

@galen-pivotal dlock approach seems too heavy-weight for updating a key. Every hash is single-threaded through this lock. I think the granularity we want is concurrent updates over different hashes, but thread-safe updates of a particular hash.

… Redis lock service

ggreen · 2017-03-21T20:52:48Z

@galen-pivotal and @metatype, please take a look at my latest change. It uses a java.util.concurrent.locks.Lock instead of a distribute lock for the hash and set synchronization.

… to correct CI issue

ggreen · 2017-04-10T17:03:48Z

@galen-pivotal and @metatype, are there any suggestions on how to move this pull request forward?

galen-pivotal · 2017-04-11T16:52:26Z

@ggreen, I haven't had a lot of time to look at this lately. It looks like you checked in some classes in the latest commit; we shouldn't have classes checked into the repository.

I'm also having trouble reading your commit history; it's hard to tell what the merge commits did, in particular d3b2642 .

If you have a Review Board account, it may be easier to read the changes as diffset, and avoids some of the git merging-develop nastiness.

It looks like there's a concurrency bug in the LockService, getting keys out of the WeakHashMap isn't synchronized. I've got a WIP at my fork, branch pr-404-concurrentLock that addresses this. I haven't had a chance to look at it in a while, though.

galen-pivotal · 2017-04-11T16:56:54Z

@ggreen If you can clean up the history from where my PR was merged this will be much easier to review. In general, even though we've added the @Experimental tag, I would like the PR to be complete and release-ready once we merge it to develop.

yhilem · 2017-05-29T08:40:21Z

Hi Team,
Would you like to integrate this PR in the next delivery please?

I want to use Geode Redis Adaptor in Conductor (https://github.com/Netflix/conductor) that enables the use of other backends implementing the database access interfaces (DAO) as discussed in this link (https://netflix.github.io/conductor/extend/).

Best regards,
Youcef HILEM

bbaynes · 2017-06-02T16:24:23Z

@yhilem @ggreen Looks like a great use case. The PR may need a little cleanup (based on Galen's comments), but it would be good to get this in.

kohlmu-pivotal · 2017-06-02T16:51:43Z

@yhilem @ggreen @bbaynes, there is currently a feature branch https://github.com/apache/geode/tree/feature/GEODE-2444, which started splitting the Redis Adapter into its own module.
I would prefer this splitting of the Redis adapter into its own module to be part of the integration.

metatype · 2017-07-18T20:18:47Z

What would it take to get this PR merged?

galen-pivotal · 2017-07-19T22:03:25Z

At this point this PR is a bit out of date. To merge, all of the .class, .pdf, .fig, etc. files would need to be removed, the merge fix done and whatever other cleanup is necessary. It also looks like there may be a concurrency bug, so that should be tested and fixed if necessary. And whatever other fixes are necessary. It's not a trivial amount of work.

galen-pivotal · 2017-12-20T19:02:48Z

I'm going to close this PR due to its age and inactivity.

- This work optimizes and simplifies storage of hashes by NOT having a separate region for every hash. Authored-by: Jens Deppe <jdeppe@pivotal.io>

… regions This PR builds on the work originally submitted by Greg Green in apache#404. Also acknowledgements to Galen O'Sullivan for addressing locking issues in that PR. This commit changes the storage model of Redis hashes and sets from one region per each Redis key to a single hash region and single set region. The Redis key is now also the region key and the data is stored in a Map and Set respectively in the region. Currently, the backing values do not implement Delta changes, however this will be a future optimization. This also fixes the inability of Redis keys to contain other characters commonly used, such as colons (':'). - Add `RedisLockService` which manages a lock per key within a single JVM. Locks are held in a WeakHasMap to allow for automatic cleanup (prior PR work, using a pure ConcurrentHashMap, ended up leaking memory since there is no straight-forward way to clean up unused keys/locks). - Add new tests including concurrency tests for hashes and sets Co-authored-by: Greg Green <ggreen@pivotal.io> Co-authored-by: Ray Ingles <ringles@pivotal.io> Co-authored-by: Sarah Abbey <sabbey@pivotal.io> Co-authored-by: John Hutchison <jhutchison@pivotal.io> Co-authored-by: Murtuza Boxwala <mboxwala@pivotal.io> Co-authored-by: Prasath Durairaj <prasathd@pivotal.io> Co-authored-by: Jens Deppe <jdeppe@pivotal.io>

… regions (#4745) * GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions This PR builds on the work originally submitted by Greg Green in #404. Also acknowledgements to Galen O'Sullivan for addressing locking issues in that PR. This commit changes the storage model of Redis hashes and sets from one region per each Redis key to a single hash region and single set region. The Redis key is now also the region key and the data is stored in a Map and Set respectively in the region. Currently, the backing values do not implement Delta changes, however this will be a future optimization. This also fixes the inability of Redis keys to contain other characters commonly used, such as colons (':'). - Add `RedisLockService` which manages a lock per key within a single JVM. Locks are held in a WeakHasMap to allow for automatic cleanup (prior PR work, using a pure ConcurrentHashMap, ended up leaking memory since there is no straight-forward way to clean up unused keys/locks). - Add new tests including concurrency tests for hashes and sets Co-authored-by: Greg Green <ggreen@pivotal.io> Co-authored-by: Ray Ingles <ringles@pivotal.io> Co-authored-by: Sarah Abbey <sabbey@pivotal.io> Co-authored-by: John Hutchison <jhutchison@pivotal.io> Co-authored-by: Murtuza Boxwala <mboxwala@pivotal.io> Co-authored-by: Prasath Durairaj <prasathd@pivotal.io> Co-authored-by: Jens Deppe <jdeppe@pivotal.io>

Gregory Green added 7 commits February 14, 2017 15:31

GEODE-2469 initial changes to support Redis objects

44b90c4

GEODE-2469 changes to corrected introduced issue found during JUNIT runs

27d7600

Merge branch 'develop' into GEODE-2469

a4ee164

GEODE-2469 add additional junit test coverage

d5191fb

GEODE-2469 add additional junit test coverage

0b3bf68

Merge branch 'develop' into GEODE-2469

1958b1e

GEODE-2469 adding licenses to fix RAT issues

628f509

bschuchardt requested changes Feb 22, 2017

View reviewed changes

GEODE-2469 missed check into for SetInterpreter

3316d25

GEODE-2469 fixing CI build issues

f2b7ee7

hiteshk25 reviewed Feb 28, 2017

View reviewed changes

hiteshk25 reviewed Mar 2, 2017

View reviewed changes

galen-pivotal reviewed Mar 2, 2017

View reviewed changes

Gregory Green added 3 commits March 6, 2017 07:00

Merge branch 'develop' into GEODE-2469

886289e

GEODE-2469 addressing pull request feedback

0327403

GEOD-2469 incorporating pull request feedback, adding hash integratio…

c29b3ca

…n tests and enhancing documentation

galen-pivotal reviewed Mar 9, 2017

View reviewed changes

galen-pivotal reviewed Mar 10, 2017

View reviewed changes

Gregory Green and others added 4 commits March 11, 2017 10:54

Merge branch 'develop' into GEODE-2469

6412412

Use DLockService in Redis adapter for Hashes and Sets.

454e09a

Merge pull request #1 from galen-pivotal/pr-404-DLock

3f8c590

Use DLockService to avoid concurrency errors.

Merge branch 'GEODE-2469' of https://github.com/ggreen/geode into GEO…

d3b2642

…DE-2469

GEODE-2469 Fixing JUNIT failure, rat violations and introducing local…

31c4c4a

… Redis lock service

ggreen closed this Mar 21, 2017

ggreen reopened this Mar 21, 2017

GEODE-2469 merge with latest changes on develop branch and attempting…

cf8f0cb

… to correct CI issue

galen-pivotal closed this Dec 20, 2017

jdeppe-pivotal mentioned this pull request Feb 28, 2020

GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions #4745

Merged

6 tasks


		String regionName = key.toString();

		// check if type is hash TODO: remove

Geode 2469 #404

Geode 2469 #404

Conversation

ggreen commented Feb 19, 2017

bschuchardt left a comment

Choose a reason for hiding this comment

ggreen commented Feb 22, 2017 via email

metatype commented Feb 25, 2017

kohlmu-pivotal commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiteshk25 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreen Mar 2, 2017 • edited

Choose a reason for hiding this comment

ggreen commented Mar 2, 2017

galen-pivotal commented Mar 2, 2017

galen-pivotal 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

galen-pivotal Mar 2, 2017 • edited

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

galen-pivotal 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

Choose a reason for hiding this comment

galen-pivotal Mar 10, 2017 • edited

Choose a reason for hiding this comment

metatype commented Mar 20, 2017

ggreen commented Mar 21, 2017

ggreen commented Apr 10, 2017

galen-pivotal commented Apr 11, 2017

galen-pivotal commented Apr 11, 2017

yhilem commented May 29, 2017 • edited

bbaynes commented Jun 2, 2017

kohlmu-pivotal commented Jun 2, 2017

metatype commented Jul 18, 2017

galen-pivotal commented Jul 19, 2017

galen-pivotal commented Dec 20, 2017

hiteshk25 left a comment •

edited

ggreen Mar 2, 2017 •

edited

galen-pivotal Mar 2, 2017 •

edited

galen-pivotal Mar 10, 2017 •

edited

yhilem commented May 29, 2017 •

edited