ISPN-8069 Clustered Lock first implementation #5513

karesti · 2017-10-12T10:24:39Z

https://issues.jboss.org/browse/ISPN-8069

Non reentrant, node owner (the others modes may come in another PR after)
Configuration cache and xml will be done when other configurations are supported
Documentation is in progress
Race conditions may exist
The underlying cache is a replicated cache for high consistency
Split brain and nodes leaving is handled but should be improved yet

karesti · 2017-10-12T10:35:12Z

@belaban your review is more than welcome ! 😄

pruivo · 2017-10-13T16:12:58Z

lock/src/main/java/org/infinispan/concurrent/api/lock/impl/lock/ClusteredLockImpl.java

+               nextRequestor = pendingRequests.poll();
+
+            final LockRequestHolder requestor = nextRequestor;
+            clusteredLockManager.execute(() -> lock(requestor));


executor can be remove once https://issues.jboss.org/browse/ISPN-8336 is fixed

There is a race condition here with tryLock() with timer.
when a lock is released, the timer can be triggered and complete the future while the lock-request is in the network or in progress.
The lock is be blocked by this node in the cache and it can only be released from a unlock invocation of this node.

pruivo · 2017-10-14T08:33:07Z

API suggestion

Add ClusterLock.isLocked() //return true if the locked is acquired by some "owner"
Add ClusterLock.amILockOwner() //returns true if the node/instance/whatever is the lock owner

basic functional tests:

2 cache managers trying to create a lock with different configs (only one should succeed)
2 cache managers retrieve the same lock (ensure it is the same lock)
2 cache managers, ensure the lock state is seen between them

other functional tests

check if a joiner doesn't reset the lock state (neither it is lost)
check if a leaver (not the lock owner) doesn't reset the lock state
check if the lock owner leave, the lock is released

Partition tests

lock stays in minority partition => no one can lock it/release it
lock stays in both partition (one copy in each partition), minority can't update. in majority
- if locked, only the owner can unlock it (if it is in the majority)
  - otherwise the lock stay locked until the merge
- if the lock is free, the majority can lock it
lock stays in majority partition => majority can progress, minority don't

Concurrency and stress test

Something like ConditionalOperationsConcurrentTest. multiple threads try to acquire the lock (tryLock()) and only one of them can succeed.
- it can be extended to remaining method lock() and tryLock(time, timeunit)

karesti · 2017-10-19T09:38:48Z

@pruivo concerning partition tests, I'm using a replicated cache. Are they still necessary in this case (beside the test that if a node leaves and the lock was acquired by this node, then it has to be released)

rvansa · 2017-10-19T09:58:31Z

lock/src/main/java/org/infinispan/lock/api/ClusteredLockManager.java

+    * @param name, the name of the lock
+    * @return {@code true} if the lock has been released
+    */
+   CompletableFuture<Boolean> release(String name);


Could we call this break, to make sure people don't use it as unlock?

Break is a reserved keyword

I renamed to forceRelease

pruivo · 2017-10-19T10:26:09Z

@karesti yes they are. If you have CP enabled (assuming you allow configure the lock as AP or CP), it would be nice to test if the partition doesn't release the lock (otherwise you will violate the consistency :))

edit: if the cache is replicated, only the second point is needed

karesti · 2017-10-24T21:43:32Z

@rvansa which was the alternative to use the randomUUID ?
@pruivo I added tests, probably more should be added. I have put a TODO in the ViewChange listener that should be fixed for rentrant locks, to avoid the following situation : View Changes, unlock is sent for a node that has leaved the cluster. Meahhinwhile this node is back and reacquires the lock. So the unlock from the view change will unlock even if the lock was correctly reacquired.

Another thing that we should consider is releasing "lock" calls after an amount of time, for example, 10 minutes. So the lock won't starve forever. In case of split-brain, all the "lock" requests should be released too (or auto-release them with the scheduler after 5 minutes waiting).

karesti · 2017-10-24T21:47:18Z

@wburns I asked for a review, if you have time, of course 💃

karesti · 2017-10-25T10:46:52Z

Tested in vert-x3/vertx-infinispan#39

rvansa · 2017-10-25T12:06:34Z

@karesti Just use ThreadLocalRandom.current().nextLong() to generate 2 random longs and then UUID(long, long) ctor. You probably don't need the UUID at all, just generate some string with enough randomness.

tristantarrant · 2017-10-25T13:00:59Z

Use Util.threadLocalRandomUUID()

tristantarrant · 2017-10-19T18:48:42Z

lock/src/main/java/org/infinispan/lock/api/ClusteredLockManager.java

+    * @param name, the name of the lock
+    * @return {@code true} if the lock has been released
+    */
+   CompletableFuture<Boolean> release(String name);


Break is a reserved keyword

tristantarrant · 2017-10-19T18:50:21Z

lock/src/main/java/org/infinispan/lock/api/ClusteredLock.java

+ * @see <a href="http://infinispan.org/documentation/">Infinispan documentation</a>
+ * @since 9.2
+ */
+public interface ClusteredLock {


I'd prefer to see this in commons, with potentially an EmbeddedClusteredLock interface in the lock module.

tristantarrant · 2017-10-19T18:51:38Z

lock/src/main/java/org/infinispan/lock/api/ClusteredLockManager.java

+ * @since 9.2
+ */
+@Experimental
+public interface ClusteredLockManager {


Move to commons, with an EmbeddedClusteredLockManager interface here.

Ok, I will move to commons those interfaces !

tristantarrant · 2017-10-19T18:52:55Z

lock/src/main/java/org/infinispan/lock/api/ClusteredLockConfiguration.java

+ * @since 9.2
+ */
+@Experimental
+public class ClusteredLockConfiguration {


Looks like configuration can be common to both embedded and remote implementations

pruivo · 2017-10-25T14:47:11Z

lock/src/main/java/org/infinispan/lock/api/impl/lock/ClusteredLockImpl.java

+            return;
+         }
+
+         if (request == null) {


bug: should result == null

Indeed ! I could not reproduce this case in tests haha thank you pointing this out.

pruivo

There is another concurrency issue with the tryLock() with timer.

This test fails:

   public void testTryLockWithTimeout11() throws Throwable {
      assertTrue(await(lock.tryLock()));
      CompletableFuture<Boolean> tryLock = lock.tryLock(1, TimeUnit.NANOSECONDS);
      System.out.println("try-lock result=" + await(tryLock));
      System.out.println("is-locked result=" + await(lock.isLocked()));
      System.out.println("is-locked-by-me result=" + await(lock.isLockedByMe()));
      assertTrue(await(tryLock));
      assertTrue(await(lock.isLocked()));
      assertTrue(await(lock.isLockedByMe()));
   }

the tryLock() reports false but the lock is acquired.

try-lock result=false
is-locked result=true
is-locked-by-me result=true

pruivo · 2017-10-25T14:50:44Z

lock/src/main/java/org/infinispan/lock/api/impl/lock/ClusteredLockImpl.java

+      @Override
+      protected void handle(Boolean result) {
+         if (!wait) {
+            log.trace("Wait is " + wait + "on request" + this);


wait is always false.
Also, start using log.tracef() when you have arguments

pruivo · 2017-10-25T14:56:09Z

lock/src/main/java/org/infinispan/lock/api/impl/lock/ClusteredLockImpl.java

+            // The answer has to be returned without holding the CompletableFuture
+            request.complete(result);
+         } else if (result) {
+            if (request.isDone()) {


you still have concurrency issues here. the schedule can be triggered between request.isDone() and request.complete(true)

my suggestion

request.complete(true) if (request.getNow(false)) unlock()

the CompletableFuture only register the first complete(). getNow() only returns false if the schedule arrived first.

@pruivo I did another thing, your comment's are isolated in a commit

pruivo · 2017-10-25T14:56:51Z

lock/src/main/java/org/infinispan/lock/api/impl/lock/ClusteredLockImpl.java

+            return;
+         }
+
+         if (request.isDone()) {


I'm not sure about this. the schedule executor can complete the request but the unlock never happens.

I have to check the status at the end

and unlock if needed

pruivo · 2017-10-25T15:01:33Z

lock/src/main/java/org/infinispan/lock/api/impl/lock/ClusteredLockImpl.java

+
+   @Override
+   public CompletableFuture<Boolean> tryLock(long time, TimeUnit unit) {
+      log.trace("tryLock with timeout (" + time + "," + unit + ") called from " + originator);


shouldn't invoke tryLock() if time <= 0?

yes, indeed.

pruivo · 2017-10-25T15:19:00Z

lock/src/test/java/org/infinispan/lock/api/ClusteredLockSplitBrainTest.java

+
+      await(lock0.lock()
+            .thenRun(() -> {
+               assertTrue("we locked with node A", true);


weird assertTrue(true)

This is just a way to document, but I can change it

I will do something smarter

pruivo · 2017-10-25T15:21:41Z

lock/src/test/java/org/infinispan/lock/api/ClusteredLockSplitBrainTest.java

+import org.testng.annotations.Test;
+
+@Test(groups = "functional", testName = "clusteredLock.ClusteredLockSplitBrainTest")
+public class ClusteredLockSplitBrainTest extends BasePartitionHandlingTest {


there are missing tests with splitting the cluster with lock acquired.
also, it would be nice to test the tryLock()s

I didn't want to go to more tests without a check on this one first.

pruivo · 2017-10-25T15:25:48Z

lock/src/main/java/org/infinispan/lock/api/impl/entries/ClusteredLockValue.java

+
+      @Override
+      public void writeObject(ObjectOutput output, ClusteredLockValue object) throws IOException {
+         output.writeObject(object.requestId);


MarshallUtil.marshallString(object.requestId, output);

pruivo · 2017-10-25T15:28:00Z

lock/src/main/java/org/infinispan/lock/api/impl/manager/EmbeddedClusteredLockManager.java

+   public boolean defineLock(String name, ClusteredLockConfiguration configuration) {
+      // TODO: Configuration is not used because we don't support any other mode for now. For that : ISPN-8413
+      CacheHolder cacheHolder = extractCacheHolder(cacheHolderFuture);
+      cache = cacheHolder.getClusteredLockCache();


can throw NullPointerException

if the cacheHolder is null you mean ?

Ok, I'm throwing an exception if the caches could not be started instead of returning null

pruivo · 2017-10-25T15:30:44Z

lock/src/main/java/org/infinispan/lock/api/impl/functions/LockFunction.java

+
+      @Override
+      public void writeObject(ObjectOutput output, LockFunction object) throws IOException {
+         output.writeObject(object.requestId);


MarshallUtil.marshallString(object.requestId, output);

karesti · 2017-10-25T16:19:38Z

@pruivo Concerning the There is another concurrency issue with the tryLock() with timer.

This test fails:

public void testTryLockWithTimeout11() throws Throwable {
assertTrue(await(lock.tryLock()));
CompletableFuture tryLock = lock.tryLock(1, TimeUnit.NANOSECONDS);
System.out.println("try-lock result=" + await(tryLock));
System.out.println("is-locked result=" + await(lock.isLocked()));
System.out.println("is-locked-by-me result=" + await(lock.isLockedByMe()));
assertTrue(await(tryLock));
assertTrue(await(lock.isLocked()));
assertTrue(await(lock.isLockedByMe()));
}
the tryLock() reports false but the lock is acquired.

try-lock result=false
is-locked result=true
is-locked-by-me result=true

This is working properly. The lock is not reentrant. The lock is locked by the first call, but the second tryLock fails because it is already acquired

pruivo · 2017-10-25T16:41:31Z

@karesti there is only one tryLock()

edit, ah the first was commented. copy-paste error. The test still fails :)

danberindei · 2017-10-31T09:16:01Z

@karesti http://ci.infinispan.org/job/Infinispan/job/PR-5513/33/testReport/junit/org.infinispan.lock/ClusteredLockSplitBrainTest/testLockCreationWhenPartitionHappening_DIST_SYNC_/

karesti · 2017-10-31T09:31:33Z

@danberindei yes, I know, split brain fails in jenkins. I have to check tests and code ... but I'm in another fight before that !!

danberindei · 2017-10-31T13:22:33Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+<dependency>
+   <groupId>org.infinispan</groupId>
+   <artifactId>infinispan-clustered-lock</artifactId>
+   <version>...</version> <!-- 9.2.0.Final or higher -->


@tristantarrant Maybe we should have a placeholder like {infinispanversion} but with the full version?

Indeed. @karesti please add a <fullinfinispanversion>${project.version}</fullinfinispanversion> to documentation/pom.xml in the <attributes> section so that we can substitute it correctly when docs are generated

but I don't want this value to be replaced each time. This is something from 9.2.0.Final. I did the same thing as in counters where it is said
I will do this in a separate PR you want

danberindei · 2017-10-31T13:47:24Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+`ClusteredLockManager.get("lockName")` is called.
+
+This means that each time we get a `ClusteredLock` instance with the `ClusteredCacheManager`, this instance will be
+a new instance.


Sorry I'm commenting so late, but I find the change in the ClusteredLockManager.get(name) behavior confusing.

Personally I like the vert.x API, where acquiring the lock gives you a Lock instance, and you can only release it through that Lock instance. Being able to release a lock that some other application component has acquired without any communication with that component sounds wrong to me.

Don't worry, input is always welcome Dan ! So as I understand, the ownership level "NODE" is not ok and we should support "INSTANCE" level locking (rentrant or not rentrant) ?

About this point, @rvansa considered that we might only need "object" level (at least for now). We discussed that briefly during the F2F

First of all, I'm not enunciating rules here, I'm just thinking aloud :) But I think even with the vert.x API, node-ownership is still be useful if the lock is reentrant: that way multiple logical threads on the same node can acquire the lock concurrently, and only the final release actually allows another node to acquire it.

Thinking about it some more, maybe we should allow the application to use its own lock owner, as in clusteredLock.lock(lockOwner) or clusteredLock.withOwner(lockOwner).lock() (to match the not-yet-public API in Cache). That way, different application components that deal with a single request could call get(name).lock(requestId) without the need to explicitly pass a lock instance between them. Regular lock() would use the local node as the lock owner.

@karesti It's you who came with the notion of node-level locks; I haven't seen that in Vert.X but the fact that the lock is not tied to a thread. To me a node-level ownership with non-reentrancy seems the same as instance-level.

@danberindei Sounds good, but the owner should be provided when the lock is retrieved from manager rather than after that (to enforce const owner by api).

@rvansa it's not accurate to say that was me who came to that idea of ownership or reentrancy levels, but this is not the discussion here.
I think there is a misunderstanding here and probably is due to something I did not fully get

@danberindei concerning the "with owner" this is something I was considering too

+1 for withOwner

danberindei · 2017-10-31T13:49:27Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+
+The `ClusteredLockManager` interface, *marked as experimental*, is the entry point to define, retrieve and remove a lock.
+It automatically listen to the creation of `EmbeddedCacheManager` and proceeds with the registration  of an
+instance of it per `EmbeddedCacheManager`.


Does the interface really listen to events? And should the user care if the lock manager is created at the same time as the cache manager, or only on the first call to EmbeddedClusteredLockManagerFactory.from(EmbeddedCacheManager)?

This is not very clear, indeed, I have to rework this part.

danberindei · 2017-10-31T13:53:55Z

lock/src/main/java/org/infinispan/lock/impl/ClusteredLockModuleLifecycle.java

+
+   private static CompletableFuture<CacheHolder> startCaches(EmbeddedCacheManager cacheManager) {
+      final CompletableFuture<CacheHolder> future = new CompletableFuture<>();
+      new Thread(() -> {


Please set a thread name that includes the node name for easier debugging.

tristantarrant

Pretty good job here. A few minor changes are needed

tristantarrant · 2017-11-08T17:05:59Z

commons/src/main/java/org/infinispan/lock/api/ClusteredLock.java

+public interface ClusteredLock {
+
+   /**
+    * Acquires the lock. If the lock is not available then the {@link CompletableFuture} waits til the lock has been acquired.


s/til/until/

tristantarrant · 2017-11-08T17:06:45Z

commons/src/main/java/org/infinispan/lock/api/ClusteredLock.java

+
+   /**
+    * If the lock is available this method returns immediately with the {@link CompletableFuture} holding the value {@code true}.
+    * If the lock is not available then the {@link CompletableFuture} waits til :


s/til/until/

tristantarrant · 2017-11-08T17:16:32Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+== Clustered Lock
+
+Clustered lock is a new building block introduced in Infinispan 9.2
+It is a lock distributed and shared among all nodes in the Infinispan cluster and currently provides a way to execute


"It is a lock which is distributed and shared..."

tristantarrant · 2017-11-08T17:19:10Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+<dependency>
+   <groupId>org.infinispan</groupId>
+   <artifactId>infinispan-clustered-lock</artifactId>
+   <version>...</version> <!-- 9.2.0.Final or higher -->


Indeed. @karesti please add a <fullinfinispanversion>${project.version}</fullinfinispanversion> to documentation/pom.xml in the <attributes> section so that we can substitute it correctly when docs are generated

tristantarrant · 2017-11-08T17:21:36Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+`ClusteredLockManager.get("lockName")` is called.
+
+This means that each time we get a `ClusteredLock` instance with the `ClusteredCacheManager`, this instance will be
+a new instance.


+1 for withOwner

tristantarrant · 2017-11-08T17:24:46Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+----
+
+* `lock` :
+Acquires the lock. If the lock is not available then call blocks til the lock is acquired.


s/til/until/

tristantarrant · 2017-11-08T17:24:55Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+
+* `tryLock(long time, TimeUnit unit)`
+If the lock is available this method returns immediately with `true`.
+If the lock is not available then the call waits til :


s/til/until/

tristantarrant · 2017-11-08T17:25:15Z

documentation/src/main/asciidoc/user_guide/clustered_lock.adoc

+Returns `true` when the lock is locked and `false` when the lock is released.
+
+* `isLockedByMe`
+Returns a `true` when the lock is owned by the caller and `false` when the lock is owned by someone else or it's released.


s/Returns a/Returns/

tristantarrant · 2017-11-08T17:27:43Z

lock/src/main/java/org/infinispan/lock/impl/ClusteredLockModuleLifecycle.java

+@MetaInfServices(value = ModuleLifecycle.class)
+public class ClusteredLockModuleLifecycle implements ModuleLifecycle {
+
+   public static final String CLUSTERED_LOCK_CACHE_NAME = "___clustered_locks";


Please use org.infinispan.LOCKS

tristantarrant · 2017-11-08T17:34:24Z

lock/src/test/java/org/infinispan/lock/ClusteredLockWith2NodesKillingOneTest.java

+
+import static org.infinispan.functional.FunctionalTestUtils.await;
+import static org.infinispan.test.TestingUtil.killCacheManagers;
+import static org.testng.Assert.assertEquals;


We tend to favor the AssertJUnit versions of these

Wrong import by mistake. Let me change and push

* Non reentrant lock * Lock Owner is the Node

karesti · 2017-11-08T19:21:11Z

@tristantarrant I reported your modifications, minus the substitution of the version at least for now. if jenkins is ok, I would like to merge

karesti · 2017-11-09T12:33:19Z

@tristantarrant jenkins seems ok !

tristantarrant · 2017-11-09T12:38:42Z

Merged. Thanks !

karesti · 2017-11-09T12:45:23Z

@tristantarrant thank you 😭

karesti added the Preview label Oct 12, 2017

karesti added this to the 9.2.0.Alpha2 milestone Oct 12, 2017

karesti force-pushed the ISPN-8069 branch from 2a3fb5b to 299f8a8 Compare October 12, 2017 10:33

karesti force-pushed the ISPN-8069 branch from 299f8a8 to 1a9342e Compare October 13, 2017 10:38

pruivo reviewed Oct 13, 2017

View reviewed changes

karesti force-pushed the ISPN-8069 branch from 9799d67 to e1a5c07 Compare October 19, 2017 09:34

rvansa reviewed Oct 19, 2017

View reviewed changes

karesti force-pushed the ISPN-8069 branch from e1a5c07 to 4088a76 Compare October 24, 2017 21:20

karesti added Ready for Review and removed Preview labels Oct 24, 2017

karesti force-pushed the ISPN-8069 branch 2 times, most recently from ac8365f to fc64234 Compare October 24, 2017 21:27

karesti requested a review from wburns October 24, 2017 21:46

karesti force-pushed the ISPN-8069 branch from fc64234 to 66f78c4 Compare October 24, 2017 22:21

tristantarrant reviewed Oct 25, 2017

View reviewed changes

pruivo reviewed Oct 25, 2017

View reviewed changes

pruivo requested changes Oct 25, 2017

View reviewed changes

karesti modified the milestones: 9.2.0.Alpha2, 9.2.0.Beta1 Oct 25, 2017

karesti removed the request for review from wburns October 26, 2017 14:38

karesti force-pushed the ISPN-8069 branch 4 times, most recently from 204c58a to 3a11488 Compare October 27, 2017 16:11

karesti added the Wait CI Results label Oct 30, 2017

karesti added Changes Required and removed Ready for Review labels Oct 31, 2017

danberindei reviewed Oct 31, 2017

View reviewed changes

karesti force-pushed the ISPN-8069 branch from 3a11488 to c6e133f Compare November 7, 2017 22:00

karesti added Ready for Review and removed Changes Required labels Nov 7, 2017

karesti force-pushed the ISPN-8069 branch 2 times, most recently from d3a96d3 to 9123700 Compare November 8, 2017 12:20

tristantarrant requested changes Nov 8, 2017

View reviewed changes

ISPN-8069 Clustered Lock first implementation

85a5b0f

* Non reentrant lock * Lock Owner is the Node

karesti force-pushed the ISPN-8069 branch from b5505fd to 85a5b0f Compare November 8, 2017 19:17

tristantarrant approved these changes Nov 9, 2017

View reviewed changes

tristantarrant merged commit 8701b5b into infinispan:master Nov 9, 2017

karesti deleted the ISPN-8069 branch February 6, 2019 11:31

ISPN-8069 Clustered Lock first implementation #5513

ISPN-8069 Clustered Lock first implementation #5513

Conversation

karesti commented Oct 12, 2017 • edited

karesti commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pruivo commented Oct 14, 2017

karesti commented Oct 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

pruivo commented Oct 19, 2017 • edited

karesti commented Oct 24, 2017

karesti commented Oct 24, 2017

karesti commented Oct 25, 2017

rvansa commented Oct 25, 2017 • edited

tristantarrant commented Oct 25, 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

pruivo 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

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

karesti commented Oct 25, 2017 • edited

pruivo commented Oct 25, 2017 • edited

danberindei commented Oct 31, 2017

karesti commented Oct 31, 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

karesti Oct 31, 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

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

tristantarrant 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

karesti commented Oct 12, 2017 •

edited

pruivo commented Oct 19, 2017 •

edited

rvansa commented Oct 25, 2017 •

edited

karesti commented Oct 25, 2017 •

edited

pruivo commented Oct 25, 2017 •

edited

karesti Oct 31, 2017 •

edited