Support DynamoDB GSI #760

jungseoklee · 2018-09-22T05:14:15Z

Changes since v2:

Not consider attributeDefinitions when counting a number of non-key attributes
Add a table only case to integration test
Rebase on top of the latest tip, v0.10.0

Changes since v1:

Validate the case where ALL or KEYS_ONLY projection type is given along with non-key attributes
Add documentations on a couple of methods
Rename a method

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Summary

This patch supports DynamoDB GSI, which is part of #655.

Test

OK: 45 assertions (310ms)

=============================== Coverage summary ===============================
Statements   : 98.17% ( 107/109 )
Branches     : 95.06% ( 77/81 )
Functions    : 100% ( 23/23 )
Lines        : 98.11% ( 104/106 )
================================================================================
Verifying integ.dynamodb.js against integ.dynamodb.expected.json... OK.

Notes

I refactored tests to maintain and generate key attributes.
I updated integ.dynamodb.expected.json through npm run integ command.

eladb · 2018-09-22T05:19:02Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+        if (props.projectionType === ProjectionType.Include && !props.nonKeyAttributes) {
+            // https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-dynamodb-projectionobject.html
+            throw new Error(`nonKeyAttributes must be specified when using ${ProjectionType.Include} projection type`);
+        }


Is also true the other way around? Should we add a validation that if nonKeyAttributes is set the projectType is not KEYS-ONLY?

I suppose that you meant Include, not KEYS-ONLY.

I think adding validation makes sense from use cases perspective. For example, ALL with non-key attributes does not give any value. I will check behavior and update this snippet accordingly.

eladb · 2018-09-22T05:21:59Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+        // throw error if key attribute is part of non-key attributes
+        this.attributeDefinitions.forEach(keyAttribute => {
+            if (typeof keyAttribute.attributeName === 'string' && this.nonKeyAttributes.includes(keyAttribute.attributeName)) {
+                throw new Error(`${keyAttribute.attributeName} should not be part of ${this.nonKeyAttributes}`);


Explain why in the error message

All key attributes are added by default when using Include projection type, and CloudFormation fails to create a stack if a key attribute is part of non-key attributes.

I will reword the error message in a more descriptive way.

eladb · 2018-09-22T05:24:22Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

            keyType
        });
        return this;
    }

-    private registerAttribute(name: string, type: AttributeType) {
+    private registerAttribute(attribute: Attribute) {


Can we rename this to “validateAttribute” or something like that?

Also add documentation explaining what this method does

I think "register" comes from the following snippet, and this.attributeDefinitions is used as an input parameter for table resource. Thus, IMHO, the original naming looks reasonable.

if (!existingDef) { this.attributeDefinitions.push({ attributeName: name, attributeType: type }); }

It is a good idea to have documentation here. I will add it.

eladb · 2018-09-22T05:25:55Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

@@ -303,6 +399,23 @@ export class Table extends Construct {
            });
        }
    }
+
+    private registerNonKeyAttributes(nonKeyAttributes: string[]) {


Rename to validateNonKeyAttribute and add docs

Sure. I will.

rix0rrr · 2018-09-26T08:02:59Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

        this.table = new dynamodb.TableResource(this, 'Resource', {
            tableName: props.tableName,
            keySchema: this.keySchema,
            attributeDefinitions: this.attributeDefinitions,
+            globalSecondaryIndexes: this.globalSecondaryIndexes,


If there are no GSIs, can this array be empty or do we have to use the value undefined (leave it out in CloudFormation?). Different resources have different rules here. I'd like to see a table without a GSI in the integ test to confirm that an empty array is okay.

An empty array works.

I agree with having an integration test. I will add table only case.

rix0rrr · 2018-09-26T08:04:02Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

            pointInTimeRecoverySpecification: props.pitrEnabled ? { pointInTimeRecoveryEnabled: props.pitrEnabled } : undefined,
-            provisionedThroughput: { readCapacityUnits, writeCapacityUnits },
+            provisionedThroughput: { readCapacityUnits: props.readCapacity || 5, writeCapacityUnits: props.writeCapacity || 5 },


Why is 5 the right default (for example, why not 1 or 10)? Can we refer to justification for this somewhere?

I do not know why it was 5 originally, but here is my guess:

A previous minimum capacity unit was 5 [1], but it is 1 now.

AWS Free Tier supported up to 5 RCU and WCU before although 25 units are available now [2].

If we change it to 1, we could rationalize it from cost perspective. I don't have any strong opinion here.

[1] https://aws.amazon.com/blogs/aws/amazon-dynamodb-reduced-minimum-throughput/
[2] https://aws.amazon.com/free/?utm_source=AWSBlog&utm_medium=socialmedia&SM_AWS_Blog_DDBMin

rix0rrr · 2018-09-26T08:05:50Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+        }
+
+        if (props.projectionType === ProjectionType.Include && !props.nonKeyAttributes) {
+            // https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-dynamodb-projectionobject.html


Thanks for including these links, that's fantastic!

rix0rrr · 2018-09-26T08:07:16Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+    private validateNonKeyAttributes(nonKeyAttributes: string[]) {
+        if (this.attributeDefinitions.length + this.nonKeyAttributes.length + nonKeyAttributes.length > 20) {
+            // https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Limits.html#limits-secondary-indexes
+            throw new RangeError('a maximum number of nonKeyAttributes across all of secondary indexes is 20');


Is it correct to include attributeDefinitions in this computation?

Thanks for the question! I tested this again and attributeDefinitions should not be there.

I will add a case where creating 20 non-key attributes to the integration test.

rix0rrr · 2018-09-26T08:07:59Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+        }
+
+        // store all non-key attributes
+        nonKeyAttributes.forEach(nonKeyAttribute => this.nonKeyAttributes.push(nonKeyAttribute));


Simpler way to write this:

this.nonKeyAttributes.push(...nonKeyAttributes);

I like it. I will update.

rix0rrr · 2018-09-27T10:11:55Z

Status update: looks like we have a couple more changes coming down the pipeline, afterwards this is ready to merge.

eladb · 2018-09-27T17:50:20Z

Guys, when you are ready with this, let me knwo and I'll help you fix the indentation issues when merging from master

This patch supports DynamoDB GSI.

jungseoklee · 2018-09-28T03:12:14Z

I have applied 2-space indentation as part of rebase.

Bug Fixes ========= * **aws-apigateway:** allow + in path parts ([#769](#769)) ([9aadcb6](9aadcb6)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([88b599d](88b599d)), closes [#787](#787) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([a09afc4](a09afc4)), closes [#721](#721) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([6c523f2](6c523f2)) * **aws-ec2:** fix typo in resource identifier ([#818](#818)) ([bebfef0](bebfef0)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([8caa28c](8caa28c)) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c21ebb5](c21ebb5)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([96767d7](96767d7)) * Emit valid YAML-1.1 ([#876](#876)) ([3cedc0c](3cedc0c)), closes [#875](#875) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([8824356](8824356)), closes [#830](#830) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([7e5738f](7e5738f)) Features ======== * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([aa76305](aa76305)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([86a55a9](86a55a9)) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([8a5299a](8a5299a)), closes [#795](#795) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([3d1095e](3d1095e)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([bba3602](bba3602)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([1561a4d](1561a4d)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([4980c97](4980c97)) * **aws-dynamodb:** tags support ([#814](#814)) ([644947a](644947a)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([a67b2d9](a67b2d9)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([8215389](8215389)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([d730ac6](d730ac6)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([56f0b4e](56f0b4e)) * Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([4e050c3](4e050c3)) * Resolve paths to nyc & nodeunit ([#887](#887)) ([66ff0a8](66ff0a8)) * upgrade to jsii v0.7.7 ([c231242](c231242))

BREAKING CHANGES ================ * The `cdk.App` initializer doesn't accept any arguments and the `app.run()` method does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); In order to interact with applications written using this version, the CDK Toolkit must also be update using: $ npm i -g aws-cdk * **aws-iam:** This change moves the `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **jsx:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. Features ======== * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([a733bd1](a733bd1)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([1ee729e](1ee729e)) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([d69b1d6](d69b1d6)), closes [#795](#795) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([dc0af46](dc0af46)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([8302541](8302541)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([f6c7760](f6c7760)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([737b481](737b481)) * **aws-dynamodb:** tags support ([#814](#814)) ([c76d8c1](c76d8c1)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([fdb4974](fdb4974)) * Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([5f2cb9f](5f2cb9f)) * Resolve paths to nyc & nodeunit ([#887](#887)) ([6d71a87](6d71a87)) * upgrade to jsii v0.7.7 ([43d2d9e](43d2d9e)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([958dce6](958dce6)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([6920b9c](6920b9c)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([50e0c41](50e0c41)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([a95f081](a95f081)) Bug Fixes ========= * **aws-apigateway:** allow + in path parts ([#769](#769)) ([6905b7e](6905b7e)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b7c244f](b7c244f)), closes [#787](#787) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([d279a1d](d279a1d)), closes [#721](#721) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([cf62e9d](cf62e9d)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([9a53069](9a53069)) * Emit valid YAML-1.1 ([#876](#876)) ([6c98b73](6c98b73)), closes [#875](#875) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c38c3e7](c38c3e7)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([be65a04](be65a04)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([2a979cc](2a979cc)), closes [#830](#830) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([1ec443e](1ec443e)) Code Refactoring ================ * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([6c58556](6c58556)), closes [#196](#196) * remove app boilerplate and improvements to cx protocol ([#868](#868)) ([7bb5a60](7bb5a60)), closes [#216](#216)

Bug Fixes --------- * **aws-apigateway:** allow + in path parts ([#769](#769)) ([0c50d27](0c50d27)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b0f3298](b0f3298)), closes [#787](#787) * **aws-cloudfront:** check for undefined and determining of the defaultRootObject prop is set or not ([#801](#801)) ([32a74c6](32a74c6)) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([5512f70](5512f70)), closes [#721](#721) * **aws-codecommit:** typo in README ([#780](#780)) ([0e79c2d](0e79c2d)) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([d36ee6d](d36ee6d)) * **aws-ec2:** fix capitalization of "VPCEndpointType" to "VpcEndpointType" ([#789](#789)) ([7a8ee2c](7a8ee2c)), closes [#765](#765) * **aws-ec2:** fix typo in resource identifier ([#818](#818)) ([f529c80](f529c80)) * **aws-elbv2:** fix load balancer registration ([#890](#890)) ([8cc9abe](8cc9abe)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([a65060d](a65060d)) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c592b7f](c592b7f)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([477c827](477c827)) * **cdk:** init templates were not upgraded to typescript ^3.0.0 ([#904](#904)) ([2cc7475](2cc7475)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([76d8031](76d8031)), closes [#830](#830) * **cfn2ts:** expect Token instead of CloudFormationToken ([#896](#896)) ([6eee1d2](6eee1d2)) * **docs:** fix issue [#718](#718) (Aurora DB example) ([#783](#783)) ([016f3a8](016f3a8)) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([ffac98c](ffac98c)) * Correct heading level of CHANGELOG.md 0.10.0 ([40d9ef0](40d9ef0)) * Emit valid YAML-1.1 ([#876](#876)) ([ff857ea](ff857ea)), closes [#875](#875) * **toolkit:** improve error message for large templates ([#900](#900)) ([a41f48f](a41f48f)), closes [#34](#34) Code Refactoring ---------------- * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([d46a95b](d46a95b)), closes [#196](#196) * **util:** remove [@aws-cdk](https://github.com/aws-cdk)/util ([#745](#745)) ([10015cb](10015cb)), closes [#709](#709) * **framework:** remove app boilerplate and improvements to cx protocol ([#868](#868)) ([005beec](005beec)), closes [#216](#216) Features -------- * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([905a95d](905a95d)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([72aa872](72aa872)) * **aws-cloudformation:** rename the CFN CodePipeline Actions. ([#771](#771)) ([007e7b4](007e7b4)) * **aws-cloudformation:** update the ReadMe of the module to reflect the new Action names. ([#775](#775)) ([6c0e75b](6c0e75b)), closes [#771](#771) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([b39bf11](b39bf11)), closes [#795](#795) * **aws-codedeploy:** Add the auto-scaling groups property to ServerDeploymentGroup. ([#739](#739)) ([0b28886](0b28886)) * **aws-codedeploy:** Deployment Configuration Construct. ([#653](#653)) ([e6b67ad](e6b67ad)) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([e7af9f5](e7af9f5)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([d146c8d](d146c8d)) * **aws-codepipeline, aws-codecommit, aws-s3:** change the convention for naming the source Actions to XxxSourceAction. ([#753](#753)) ([9c3ce7f](9c3ce7f)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([c5a4200](c5a4200)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([3601440](3601440)) * **aws-dynamodb:** tags support ([#814](#814)) ([924c84e](924c84e)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([3175af3](3175af3)) * **aws-ec2:** add support for ICMP protocol's classification Types & Codes to SecurityGroupRule ([#893](#893)) ([85bd3c0](85bd3c0)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([8ec761c](8ec761c)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([b42ef90](b42ef90)) * **aws-elasticloadbalancingv2:** support for ALB/NLB ([#750](#750)) ([bd9ee01](bd9ee01)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([bdee191](bdee191)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([1f717e1](1f717e1)) * **cdk:** configurable default SSM context provider ([#889](#889)) ([353412b](353412b)) * **core:** resource overrides (escape hatch) ([#784](#784)) ([5054eef](5054eef)), closes [#606](#606) * **aws-codepipeline**: Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([4c69118](4c69118)) * **toolkit:** Stop creating 'empty' stacks ([#779](#779)) ([1dddd8a](1dddd8a)) * **aws-autoscaling, aws-ec2:** Tagging support for AutoScaling/SecurityGroup ([#766](#766)) ([3d48eb2](3d48eb2)) ### BREAKING CHANGES * **framework:** The `cdk.App` constructor doesn't accept any arguments, and `app.run()` does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); * **framework:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. * **aws-iam:** `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes moved from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **aws-codepipeline-api**: `Artifact.subartifact` method of the CodePipeline API was renamed to `Artifact.atPath`. * constructor signature of `TagManager` has changed. `initialTags` is now passed inside a props object. * **util:** @aws-cdk/util is no longer available * **aws-elasticloadbalancingv2:** Adds classes for modeling Application and Network Load Balancers. AutoScalingGroups now implement the interface that makes constructs a load balancing target. The breaking change is that Security Group rule identifiers have been changed in order to make adding rules more reliable. No code changes are necessary but existing deployments may experience unexpected changes. * **aws-cloudformation:** this renames all CloudFormation Actions for CodePipeline to bring them in line with Actions defined in other service packages. * **aws-codepipeline, aws-codecommit, aws-s3:** change the names of the source Actions from XxxSource to XxxSourceAction. This is to align them with the other Actions, like Build. Also, CodeBuild has the concept of Sources, so it makes sense to strongly differentiate between the two.

akozlov75 · 2020-09-15T10:52:43Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

    return this;
  }

+  public addGlobalSecondaryIndex(props: SecondaryIndexProps) {
+    if (this.globalSecondaryIndexes.length === 5) {


According to this document https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Limits.html#limits-secondary-indexes
Number should be 20

In the latest version there is no limit at all. https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-dynamodb/lib/table.ts

eladb approved these changes Sep 22, 2018

View reviewed changes

rix0rrr reviewed Sep 26, 2018

View reviewed changes

Support DynamoDB GSI

737010e

This patch supports DynamoDB GSI.

rix0rrr merged commit 4980c97 into aws:master Sep 28, 2018

eladb mentioned this pull request Oct 10, 2018

v0.11.0 #888

Merged

speller26 mentioned this pull request Oct 7, 2019

Cannot use key attribute in one DynamoDB index as non-key attribute in another index #4398

Closed

jungseoklee deleted the for-dynamodb-gsi branch February 2, 2020 01:08

akozlov75 reviewed Sep 15, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DynamoDB GSI #760

Support DynamoDB GSI #760

jungseoklee commented Sep 22, 2018 •

edited

eladb Sep 22, 2018

jungseoklee Sep 22, 2018

eladb Sep 22, 2018

jungseoklee Sep 22, 2018

eladb Sep 22, 2018

jungseoklee Sep 22, 2018

eladb Sep 22, 2018

jungseoklee Sep 22, 2018

rix0rrr Sep 26, 2018

jungseoklee Sep 27, 2018

rix0rrr Sep 26, 2018

jungseoklee Sep 27, 2018

rix0rrr Sep 26, 2018

jungseoklee Sep 27, 2018

rix0rrr Sep 26, 2018

jungseoklee Sep 27, 2018

rix0rrr Sep 26, 2018

jungseoklee Sep 27, 2018

rix0rrr commented Sep 27, 2018

eladb commented Sep 27, 2018

jungseoklee commented Sep 28, 2018

akozlov75 Sep 15, 2020

hoegertn Sep 15, 2020

Support DynamoDB GSI #760

Support DynamoDB GSI #760

Conversation

jungseoklee commented Sep 22, 2018 • edited

Summary

Test

Notes

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

rix0rrr commented Sep 27, 2018

eladb commented Sep 27, 2018

jungseoklee commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungseoklee commented Sep 22, 2018 •

edited