DistributionSchedule uses custom wrapped int[] rather than HashSet #657

ivankelly · 2017-10-24T18:11:07Z

This avoids the autoboxing on Integers and allocations of cells for
the hashset.

This also implies using wrapped int[] object for the write set in the
DistributionSchedules and PlacementPolicies.

This patch was originally submitted as dc7933b on the yahoo-4.3
branch, though this has been modified extensively to remove the
dependency on carrotsearch hppc and to allow it to work with placement
policies which didn't exist at the time of the original patch.

eolivelli

This is a great change. And I understand the benefit. Please take into account the possibility to not introduce a new client side dependency

eolivelli · 2017-10-24T18:23:26Z

bookkeeper-server/pom.xml

@@ -178,6 +178,11 @@
      <artifactId>http-server</artifactId>
      <version>${project.version}</version>
    </dependency>
+    <dependency>
+      <groupId>com.carrotsearch</groupId>


Do you have checked license for this?
Is it really needed? We already have several custom collections, maybe we could have our own implementation of IntArrayList without importing a whole third party lib. This will be used on client side.
It is used in EnsemblePlacement policy which is very often customized, at least in my projects.
I would not like to impose such dep on public plugins API

second to this comment here:

for license, we need to update src/main/resources/NOTICE.bin.txt

it is a new dependency introduced on client. It would prefer if we can avoid using this dependency.

I am not sure what would be the benefits of using IntArrayList. If boxing, unboxing Integer and using an ArrayList is a problem, why can't we use int[] because we don't need resizing ability? we can wrap int[] into a recycle class, it should be simple and efficient, no?

Do you have checked license for this?

License is Apache v2.

I am not sure what would be the benefits of using IntArrayList. If boxing, unboxing Integer and using an ArrayList is a problem, why can't we use int[] because we don't need resizing ability? we can wrap int[] into a recycle class, it should be simple and efficient, no?

Yes, basically the class is a wrapper on top of an int[]. I guess the option here would be to either reimplement the "set" logic on the int[] or maybe import just the needed class.

We already have several custom collections, maybe we could have our own implementation of IntArrayList without importing a whole third party lib

Yes, the other collections were more about having no-boxing and concurrent collections. For non-concurrent primitive collections there are many available implementations (like the HPPC here).

eolivelli · 2017-10-24T18:25:21Z

...er-server/src/main/java/org/apache/bookkeeper/bookie/LocalBookieEnsemblePlacementPolicy.java

@@ -88,14 +89,16 @@ public BookieSocketAddress replaceBookie(int ensembleSize, int writeQuorumSize,
    }

    @Override
-    public List<Integer> reorderReadSequence(ArrayList<BookieSocketAddress> ensemble, List<Integer> writeSet,
-                                             Map<BookieSocketAddress, Long> bookieFailureHistory) {
+    public IntArrayList reorderReadSequence(


We are breaking compatibility. We already did break compatibility for policy in 4.5....
Anyway if you have I would like to have a BK class

I am fine with this, if it helps with performance - because the stability of this interface is Evolving. so it is okay to break the compatibility in minor release: https://github.com/apache/bookkeeper/blob/master/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/annotation/InterfaceStability.java#L41

https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/EnsemblePlacementPolicy.java#L195

asfgit · 2017-10-24T19:22:10Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/bookkeeper-precommit-pullrequest-docker/142/

sijie · 2017-10-24T21:34:52Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java

    private final static Logger LOG = LoggerFactory.getLogger(PendingAddOp.class);

    ByteBuf toSend;
    AddCallback cb;
    Object ctx;
    long entryId;
    int entryLength;
-    Set<Integer> writeSet;
+    IntHashSet writeSet;


Technically I think we don't need Set here. I bet an int[] would work here as well.

ivankelly · 2017-10-25T06:27:11Z

Looking at this again, bringing in an outside library is overkill. I'm going to try it with just a raw int[]. The placement policy stuff needs to be rewritten as well. It came in after the yahoo changes were made, and undoes a lot of the good work on the read path. @merlimat @sijie @eolivelli

sijie · 2017-10-25T07:25:06Z

@ivankelly what changes were undoed by the placement policy in the master? the reorder logic in placement policy isn't enabled by default, and reorder actually provides better latency in terms of aware of location distance. https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java#L97

ivankelly · 2017-10-25T07:31:54Z

Ah, i didn't see it was optional. If enabled, it causes a bunch of allocations. Perhaps this can be ignored for now. This is more important for the write path, and that's allocated per entry, whereas on read it is per block.

ivankelly · 2017-10-25T07:34:24Z

Actually, reading again, it is per entry on read too. Will think about what the best way to proceed is.

This avoids the autoboxing on Integers and allocations of cells for the hashset. This also implies using int[] for the write set in the DistributionSchedules and PlacementPolicies. This patch was originally submitted as dc7933b on the yahoo-4.3 branch, though this has been modified extensively to remove the dependency on carrotsearch hppc and to allow it to work with placement policies which didn't exist at the time of the original patch.

ivankelly · 2017-10-25T19:21:52Z

@sijie @merlimat @eolivelli This is ready for review again. Using exclusively int[] now. Removed all allocations from the read reordering also.

asfgit · 2017-10-26T05:24:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/bookkeeper-precommit-pullrequest-docker/148/

eolivelli

+1 looks good.
I have left some minor comment.

eolivelli · 2017-10-26T05:59:38Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java

@@ -92,7 +93,8 @@ PendingAddOp enableRecoveryAdd() {

    void setEntryId(long entryId) {
        this.entryId = entryId;
-        writeSet = new HashSet<Integer>(lh.distributionSchedule.getWriteSet(entryId));


If you move the allocation to the constructor w will have a write set full of zeroes which could appear valid.
In previous code an NPE would occur in case of missed call to setentryid. Recently I caught such kind of error while playing with ledger handleadv.

Good point, updating

eolivelli · 2017-10-26T06:04:21Z

...eper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicy.java

+            ArrayList<BookieSocketAddress> ensemble,
+            Map<BookieSocketAddress, Long> bookieFailureHistory,
+            int[] writeSet) {
+        return super.reorderReadLACSequence(ensemble, bookieFailureHistory,


Why do we need to call super? We can just not override the method

I left it as it always was.

eolivelli · 2017-10-26T06:10:19Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ArrayUtils2Test.java

+ */
+package org.apache.bookkeeper.util.collections;
+
+import java.util.Arrays;


Nit: reorder imports?

Updated, though I'm not sure what order you want (I don't use an IDE).

@ivankelly for me is OK, I think that we have no clear rules about import order, maybe we should discuss on the list. Maybe checkstyle will impose some ordering

//cc @sijie

Ya, a checkstyle would be good. However turning it on would result in a bunch of errors straight away. Personally, I have little preference for imports, as long as they look neat.

there is already a checkstyle rule. it applies on the modules that have checkstyle enabled. the packages in bookkeeper-server are not enforced yet.

Import Static and then Import. And sort the imports alphabetically. try to avoid start import.

<module name="ImportOrder"> <property name="severity" value="error"/>  <property name="option" value="top"/> <property name="sortStaticImportsAlphabetically" value="true"/> <property name="tokens" value="STATIC_IMPORT, IMPORT"/> <message key="import.ordering" value="Import {0} appears after other imports that it should precede"/> </module> <module name="AvoidStarImport"> <property name="severity" value="error"/> </module>

eolivelli · 2017-10-26T15:27:08Z

I think the patch is ready. I will wait for the checks.

we need to wait for @sijie final review

ivankelly · 2017-10-26T15:57:29Z

retest
previous seems stuck

sijie · 2017-10-26T18:07:59Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ArrayUtils2.java

+import java.util.Random;
+import org.apache.commons.lang3.ArrayUtils;
+
+public class ArrayUtils2 {


any reason why it is ArrayUtils2 not ArrayUtils?

also this class doesn't sound like a collection. it should probably in util package, rather than here though.

should we also move this code to bookkeeper common module ?

nah, noone of these are generic enough. Moving into the WriteSet wrapper class anyhow.

sijie · 2017-10-26T18:08:22Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ArrayUtils2.java

+import java.util.Random;
+import org.apache.commons.lang3.ArrayUtils;
+
+public class ArrayUtils2 {


Can you add at least a class comment?

sijie · 2017-10-26T18:15:38Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ArrayUtils2.java

+public class ArrayUtils2 {
+    static Random r = new Random();
+
+    public static int[] addMissingIndices(int[] array, int maxIndex) {


it would be good to add a comment on this method to explain what it does here.

all the utils here are not generic enough as an array util. it is more like distribution schedule util, because it is more specific to the way we do round-robin striping in ensemble. so I would suggest moving it into util package and renaming it as DistributionScheduleUtils and have some better names for those methods.

if we call this DistributionScheduleUtils maybe it is better to keep them in bookkeeper-server module

sijie · 2017-10-26T18:19:43Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java

-                    BookieSocketAddress addr = curEnsemble.get(bi);
-                    bookieClient.readEntry(addr, lh.getId(),
-                                           entryToRead, eecb, null);
+                final ArrayList<BookieSocketAddress> curEnsembleFinal = curEnsemble;


I am wondering why do you do this assignment here? It doesn't seem required to me.

Not needed. Got pulled in as part of cherry-pick, will remove.

Pulled in as part of cherry-pick. I guess there was a callback here at some point which required the final that isn't there now. removed.

sijie · 2017-10-26T18:20:46Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java

+
+        writeSet = new int[lh.metadata.getWriteQuorumSize()];
+        // require writeSet be populated for it to be usable
+        Arrays.fill(writeSet, -12345);


define a CONSTANT for -12345.

sijie · 2017-10-26T18:34:12Z

...keeper-server/src/main/java/org/apache/bookkeeper/client/RoundRobinDistributionSchedule.java


+    private final int[][] writeSets;


I am not sure if it is a good idea to do so. it is kind of changing the garbage collection behavior. you are going to maintain ensmebleSize * writeQuorumSize ints per ledger. if your client has N ledgers, that is going to be N * ensembleSize * writeQuorumSize, those memory are going to sit in old gen for a long time.

if you allocate int[] on getWriteSet, it is one-time use and the memory can be reclaimed on young gen gc.

If you want to avoid allocation, you can wrap int[] into a WriteSet class and only retrieve a WriteSet and set the values rather than preallocating all the write sets.

I can see other benefits when wrapping int[] into a WriteSet:

ensemble placement policy doesn't have to deal with int[] directly. it can deal with WriteSet.

all the util functions related int[] can be potentially be part of WriteSet.

Wrapping is a good idea. Doing that. Will have a knock on effect to the rest of the comments.

sijie · 2017-10-26T19:47:35Z

...keeper-server/src/main/java/org/apache/bookkeeper/client/RoundRobinDistributionSchedule.java

@@ -136,6 +221,7 @@ public QuorumCoverageSet getCoverageSet() {

    @Override
    public boolean hasEntry(long entryId, int bookieIndex) {
-        return getWriteSet(entryId).contains(bookieIndex);
+        int[] set = writeSets[(int) (entryId % ensembleSize)];


I think this can be further improved. because it is just checking whether an entry is supposed to be stored in one bookie. I think we can simply calculate it.

sijie · 2017-10-26T19:51:02Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ArrayUtils2Test.java

+ */
+package org.apache.bookkeeper.util.collections;
+
+import java.util.Arrays;


there is already a checkstyle rule. it applies on the modules that have checkstyle enabled. the packages in bookkeeper-server are not enforced yet.

Import Static and then Import. And sort the imports alphabetically. try to avoid start import.

<module name="ImportOrder"> <property name="severity" value="error"/>  <property name="option" value="top"/> <property name="sortStaticImportsAlphabetically" value="true"/> <property name="tokens" value="STATIC_IMPORT, IMPORT"/> <message key="import.ordering" value="Import {0} appears after other imports that it should precede"/> </module> <module name="AvoidStarImport"> <property name="severity" value="error"/> </module>

sijie · 2017-10-26T19:56:53Z

...er-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java

-            List<Integer> unAvailableList = new ArrayList<Integer>(writeSet.size());
-            for (Integer idx : writeSet) {
+
+            int localMask      = 0x01 << 24;


all these can be constants?

sijie · 2017-10-26T19:59:52Z

...er-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java

@@ -500,19 +504,26 @@ protected BookieNode replaceFromRack(BookieNode bookieNodeToReplace,
    }

    @Override
-    public final List<Integer> reorderReadSequence(ArrayList<BookieSocketAddress> ensemble, List<Integer> writeSet, Map<BookieSocketAddress, Long> bookieFailureHistory) {


It is worth adding a BC test comparing the logic here with the logic in 4.5.0. they should be the same.

The tests that are already there effectively test B.C. as they put a write set through a reorder and check if it matches a set value.

This object is pooled using a netty recycler.

asfgit · 2017-10-27T17:27:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/bookkeeper-precommit-pullrequest-docker/157/

ivankelly · 2017-10-27T18:55:16Z

Not ready for review yet. Still some comments to address.

ivankelly · 2017-10-27T21:25:15Z

@sijie @eolivelli ready for review again. There's a lot of changes for the changing to a wrapped write set. Multiple patches, though only the first one is big.

sijie

great work! @ivankelly

sijie · 2017-10-30T00:02:12Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java

@@ -323,7 +327,12 @@ private synchronized int getNextReplicaIndexToReadFrom() {
        }

        private int getReplicaIndex(int bookieIndex) {
-            return writeSet.indexOf(bookieIndex);
+            for (int i = 0; i < writeSet.size(); i++) {


make this as a method of WriteSet?

writeSet already has indexOf. Removing the whole method.

sijie · 2017-10-30T00:03:01Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java

+        // Iterate over set and trigger the sendWriteRequests
+        DistributionSchedule.WriteSet writeSet
+            = lh.distributionSchedule.getWriteSet(entryId);
+        for (int i = 0; i < writeSet.size(); i++) {


try { ... } finally { writeSet.recycle(); }

eolivelli · 2017-10-30T09:52:45Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/EnsemblePlacementPolicy.java

-    public List<Integer> reorderReadLACSequence(ArrayList<BookieSocketAddress> ensemble,
-                                                List<Integer> writeSet,
-                                                Map<BookieSocketAddress, Long> bookieFailureHistory);
+    public DistributionSchedule.WriteSet reorderReadLACSequence(


we could use 'void' as return type:
we are changing the signature and we do not want client code to allocate WriteSet but we would like the given WriteSet to be modified.

Anyway I am fine with this version.
But I would like to know @ivankelly and @sijie options

@eolivelli I think void would make it a little harder to use.
For example, in a lot of places we do

WriteSet reordered = policy.reorderReadSequnce(ensemble, failed, schedule.getWriteSet(eid))

If reorder returned void, then we would need to first assign the write set returned from getWriteSet to a variable and then pass it in.

@ivankelly I see, thank you for your clarification.

eolivelli

+1 great work Ivan

This avoids the autoboxing on Integers and allocations of cells for the hashset. This also implies using wrapped int[] object for the write set in the DistributionSchedules and PlacementPolicies. This patch was originally submitted as dc7933b on the yahoo-4.3 branch, though this has been modified extensively to remove the dependency on carrotsearch hppc and to allow it to work with placement policies which didn't exist at the time of the original patch. Author: Ivan Kelly <ivank@apache.org> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes apache#657 from ivankelly/yahoo-backports

…onfirmedAndEntry Descriptions of the changes in this PR: *Motivation* There are two bugs in `ReadLastAddConfirmedAndEntry`: 1) a regression was introduced by #657. the long poll read op is attempting to long-poll reading lac. since lac is stored across ensemble, so the retry logic assumes it will attempt over all the bookies in the ensemble. however #657 use a `write-quorum-size` write set for tracking those attempts. this leads to ArrayIndexOutOfBoundsException reported at #1403. The integrate tests added in this PR can easily reproduce this issue. 2) there was a bug on retry logic when a ledger whose ensemble size is larger than write quorum size. when this happens, it will claim lac is not advanced prior to attempt the bookie in the ensemble. so the client will never know lac is advanced if using long poll reads on such ledgers. The integrate tests added in this PR can also catch this issue. disclaim: twitter uses long poll reads but never uses `ensembleSize > writeQuorumSize`. so this is not a problem for dlog users. *Solution* - introduce a `getWriteSetForLongPoll` call that uses `ensembleSize` for building the write set. this would address problem 1) - fix the assignment of `numEmptyResponsesAllowed`, so the long poll reads can work with `ensembleSize > writeQuorumSize` - add integration tests for long polling reads at the same time, also add an integration test for normal tailing reads with Author: Sijie Guo <sijie@apache.org> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None> This closes #1404 from sijie/longpoll_tests, closes #1403

…onfirmedAndEntry Descriptions of the changes in this PR: *Motivation* There are two bugs in `ReadLastAddConfirmedAndEntry`: 1) a regression was introduced by #657. the long poll read op is attempting to long-poll reading lac. since lac is stored across ensemble, so the retry logic assumes it will attempt over all the bookies in the ensemble. however #657 use a `write-quorum-size` write set for tracking those attempts. this leads to ArrayIndexOutOfBoundsException reported at #1403. The integrate tests added in this PR can easily reproduce this issue. 2) there was a bug on retry logic when a ledger whose ensemble size is larger than write quorum size. when this happens, it will claim lac is not advanced prior to attempt the bookie in the ensemble. so the client will never know lac is advanced if using long poll reads on such ledgers. The integrate tests added in this PR can also catch this issue. disclaim: twitter uses long poll reads but never uses `ensembleSize > writeQuorumSize`. so this is not a problem for dlog users. *Solution* - introduce a `getWriteSetForLongPoll` call that uses `ensembleSize` for building the write set. this would address problem 1) - fix the assignment of `numEmptyResponsesAllowed`, so the long poll reads can work with `ensembleSize > writeQuorumSize` - add integration tests for long polling reads at the same time, also add an integration test for normal tailing reads with Author: Sijie Guo <sijie@apache.org> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None> This closes #1404 from sijie/longpoll_tests, closes #1403 (cherry picked from commit 9259173) Signed-off-by: Sijie Guo <sijie@apache.org>

…stAddConfirmedAndEntry Descriptions of the changes in this PR: *Motivation* There are two bugs in `ReadLastAddConfirmedAndEntry`: 1) a regression was introduced by apache#657. the long poll read op is attempting to long-poll reading lac. since lac is stored across ensemble, so the retry logic assumes it will attempt over all the bookies in the ensemble. however apache#657 use a `write-quorum-size` write set for tracking those attempts. this leads to ArrayIndexOutOfBoundsException reported at apache#1403. The integrate tests added in this PR can easily reproduce this issue. 2) there was a bug on retry logic when a ledger whose ensemble size is larger than write quorum size. when this happens, it will claim lac is not advanced prior to attempt the bookie in the ensemble. so the client will never know lac is advanced if using long poll reads on such ledgers. The integrate tests added in this PR can also catch this issue. disclaim: twitter uses long poll reads but never uses `ensembleSize > writeQuorumSize`. so this is not a problem for dlog users. *Solution* - introduce a `getWriteSetForLongPoll` call that uses `ensembleSize` for building the write set. this would address problem 1) - fix the assignment of `numEmptyResponsesAllowed`, so the long poll reads can work with `ensembleSize > writeQuorumSize` - add integration tests for long polling reads at the same time, also add an integration test for normal tailing reads with Author: Sijie Guo <sijie@apache.org> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None> This closes apache#1404 from sijie/longpoll_tests, closes apache#1403 (cherry picked from commit 9259173) Signed-off-by: Sijie Guo <sijie@apache.org> Signed-off-by: JV Jujjuri <vjujjuri@salesforce.com>

…o issues like connection loss Quick fix to make the test more reliable. Author: Andor Molnar <andor@apache.org> Reviewers: fangmin@apache.org, eolivelli@gmail.com, andor@apache.org Closes apache#657 from anmolnar/ZOOKEEPER-3157

ivankelly requested review from sijie and eolivelli October 24, 2017 18:12

ivankelly assigned merlimat and ivankelly Oct 24, 2017

eolivelli reviewed Oct 24, 2017

View reviewed changes

sijie reviewed Oct 24, 2017

View reviewed changes

ivankelly force-pushed the yahoo-backports branch from 7769023 to a4481d0 Compare October 25, 2017 19:19

ivankelly changed the title ~~Use hash set with native ints for write set tracking~~ DistributionSchedule uses int[] rather than HashSet Oct 25, 2017

eolivelli approved these changes Oct 26, 2017

View reviewed changes

ivankelly added 2 commits October 26, 2017 08:35

@eolivelli review comments

14881dc

Forgot to import Arrays

b55b024

sijie reviewed Oct 26, 2017

View reviewed changes

Wrap the write set int[] in an object

0e023d9

This object is pooled using a netty recycler.

ivankelly added 3 commits October 27, 2017 23:10

Recycle writeSets after use

0b8fee3

Define constants for Masks in placement policies

0e978cf

Remove write set precomputation

60c6147

sijie approved these changes Oct 30, 2017

View reviewed changes

ivankelly added 2 commits October 30, 2017 09:04

Merge remote-tracking branch 'origin/master' into yahoo-backports

fe1b95d

Address @sijie review comments

ee418ee

eolivelli reviewed Oct 30, 2017

View reviewed changes

eolivelli approved these changes Oct 30, 2017

View reviewed changes

ivankelly changed the title ~~DistributionSchedule uses int[] rather than HashSet~~ DistributionSchedule uses custom wrapped int[] rather than HashSet Oct 30, 2017

ivankelly added this to the 4.6.0 milestone Oct 30, 2017

ivankelly added area/client release/4.6.0 type/task labels Oct 30, 2017

ivankelly closed this in 8e20a80 Oct 30, 2017

This was referenced Nov 1, 2017

DistributionSchedule.WriteSet cannot be converted to List apache/distributedlog#233

Closed

Issue 233: DistributionSchedule.WriteSet cannot be converted to List apache/distributedlog#235

Merged

This was referenced May 12, 2018

ArrayIndexOutOfBoundsException is thrown on readLastAddConfirmedAndEntry #1403

Closed

Issue 1403: ArrayIndexOutOfBoundsException is thrown on readLastAddConfirmedAndEntry #1404

Closed

DistributionSchedule uses custom wrapped int[] rather than HashSet #657

DistributionSchedule uses custom wrapped int[] rather than HashSet #657

Conversation

ivankelly commented Oct 24, 2017 • edited Loading

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfgit commented Oct 24, 2017

Choose a reason for hiding this comment

ivankelly commented Oct 25, 2017 • edited Loading

sijie commented Oct 25, 2017

ivankelly commented Oct 25, 2017

ivankelly commented Oct 25, 2017

ivankelly commented Oct 25, 2017

asfgit commented Oct 26, 2017

eolivelli 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

eolivelli commented Oct 26, 2017

ivankelly commented Oct 26, 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

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

asfgit commented Oct 27, 2017

ivankelly commented Oct 27, 2017

ivankelly commented Oct 27, 2017

sijie 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

eolivelli left a comment

Choose a reason for hiding this comment

ivankelly commented Oct 24, 2017 •

edited

Loading

merlimat Oct 24, 2017 •

edited

Loading

ivankelly commented Oct 25, 2017 •

edited

Loading