Support alibaba cloud tablestore #16151

xumingming · 2021-05-25T02:20:34Z

Test plan - (Please fill in how you tested your changes)

Tested on my desktop with real config.

== RELEASE NOTES ==

General Changes

*  Add a new connector: tablestore for Alibaba Cloud's Tablestore service

beinan

Sorry for my delay. Really appreciate your contribution! Just a few coding style issues. (I might more focus on the coding style in the first round review. ).

This is really a big one, I will continue the review by the end of this week. Again, thank you for the contribution! Feel free to ping me via slack or reply the comments here if you have any questions or concern.

beinan · 2021-06-01T03:03:03Z

presto-tablestore/pom.xml

+	<build>
+		<plugins>
+			<plugin>
+				<groupId>org.apache.maven.plugins</groupId>


indentation style looks not following other sections

beinan · 2021-06-01T03:05:31Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/IndexFirstSet.java

+{
+    enum Type
+    {
+        auto,


I think we prefer using upper case for enum

beinan · 2021-06-01T03:10:20Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/IndexFirstSet.java

+                    .collect(Collectors.toSet());
+            return IndexFirstSet.custom(tables);
+        }
+        String str = "use hint like these: " +


thinking if we could extract 'str' as an constant
can we have a meaningful name rather than 'str'?

beinan · 2021-06-01T03:17:08Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/IndexFirstSet.java

+    private static SchemaTableName checkAndAssemblySchemaTableName(Optional<String> currentConnectionSchema,
+            String hintKey,
+            String unit)
+            throws IllegalArgumentException


hmmm, why we need explicitly throws a runtime exception in a private method?

You mean the exception should be throw in a public method?

Not really, I'm just curious

Why we need a "throws" for a RuntimeException? We don't have to, am I right? (IllegalArgumentException is a runtime/unchecked exception, isn't it?)

In case you just want to use 'throws' to make a strong contract to the caller, but 'checkAndAssemblySchemaTableName' is a private method. I cannot see the benefits of the 'throws' here.

beinan · 2021-06-01T03:18:56Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/ReflectionUtils.java

+
+    public static Field findField(Class<?> clazz, String name, Class<?> type)
+    {
+        checkArgument(clazz != null, "Class must not be null");


we might prefer to use requireNonNull instead of checkArgument here

beinan · 2021-06-01T03:35:14Z

...-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreAbstractRecordCursor.java

+                long delta = System.nanoTime() - t1;
+
+                if (delta >= 10 * 1_000_000) {
+                    log.debug("split scan new fetch, queryId=%s currentRows=%s deltaApiCost=%sms", queryId, rows, delta / 1000_1000.0);


why 1000_1000.0 ?

beinan · 2021-06-01T03:36:39Z

...-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreAbstractRecordCursor.java

+                next(); //有可能报错
+                long delta = System.nanoTime() - t1;
+
+                if (delta >= 10 * 1_000_000) {


Why we have 10 times 1_000_000? should we have a constant here?

added a constant.

It is to make sure we only log every 10ms -- so it will not generate too much logs.

beinan · 2021-06-01T03:38:24Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreConnector.java

+public class TablestoreConnector
+        implements Connector
+{
+    private static final Logger LOG = Logger.get(TablestoreConnector.class);


I might prefer using lower case 'log'

beinan · 2021-06-01T03:44:11Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreFacade.java

+        catch (Exception e) {
+            type = "others_error";
+            cause = e;
+            invalidate = true; //有可能客户端被关闭之类的情况


What happens if the client has been shutdown?
What's 'invalidate' for? I couldn't find any usage in this class.

Indeed not needed, will delete it.

beinan · 2021-06-01T03:46:44Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreFacade.java

+                else {
+                    x = querySplitsFromSearchIndex(session, th, constraint, tupleDomainStr, matchedSearchIndexes);
+                    t1 = currentTimeMillis() - t1;
+                    log.info("getSplits() end, found %s indexes, query=%s splitsSize=%s apiCost=%sms.",


Will this generate too many logs?

If the splits size and t1 are important can we expose them by jmx?

it only generate one log per query.

beinan · 2021-06-01T03:54:14Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreConnectorFactory.java

+
+    public TablestoreConnectorFactory(String name, ClassLoader classLoader)
+    {
+        checkArgument(!isNullOrEmpty(name), "name is null or empty");


Using requireNonNull

beinan · 2021-06-01T03:56:57Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreFacade.java

+    }
+
+    /**
+     * 查看总行数的查询


I think this comment is not needed. The name of this method explains the purpose well.

beinan · 2021-06-01T03:57:54Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreFacade.java

+            Iterator<Queue<Split>> i = ss.iterator();
+            while (i.hasNext()) {
+                Queue<Split> x = i.next();
+                Split e = x.poll(); //移除元素


comment not needed

beinan · 2021-06-01T03:58:03Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreFacade.java

+                    newList.add(e);
+                }
+                else {
+                    i.remove(); //移除队列


comment not needed

beinan · 2021-06-01T04:00:23Z

...blestore/src/main/java/com/facebook/presto/tablestore/TablestoreRecordCursor4RangeBlock.java

+    @Override
+    protected void next()
+    {
+        rowIterator.next(); //有可能报错


remove the comment. we can add a try/catch if you think the exception should be handled here.

beinan · 2021-06-01T04:02:25Z

...-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreRowBasedRecordCursor.java

+    @Override
+    protected void next()
+    {
+        current = rowIterator.next(); //有可能报错


comment not needed

beinan · 2021-06-01T04:04:56Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreValueUtil.java

+        switch (target.getType()) {
+            case INTEGER:
+                long v1 = target.asLong();
+                if (v1 == Long.MAX_VALUE) {


static import Long.MAX_VALUE

beinan · 2021-06-01T04:05:34Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TypeUtil.java

+    {
+        if (value == null) {
+            String str = format("We need a nonnull instance of type[%s], but it's null.", target.getSimpleName());
+            throw new NullPointerException(str);


Using requireNonNull?

beinan · 2021-06-01T04:06:25Z

presto-tablestore/src/test/java/com/facebook/presto/tablestore/TablestoreConstants.java

+    static final String COLUMN_NAME_2 = "test_column2";
+    static final String COLUMN_NAME_3 = "test_column3";
+
+    static final String PK_1_int = "pk1_int";


All upper case for a constant

beinan · 2021-06-01T04:08:42Z

presto-tablestore/src/test/java/com/facebook/presto/tablestore/TestingConnectorSession.java

+import static java.util.Locale.ENGLISH;
+import static java.util.Objects.requireNonNull;
+
+public class TestingConnectorSession


I think there has been a TestingConnectorSession in com.facebook.presto.testing, do we really need to create a new one?

Removed tablestore's TestingConnectorSession and reuse presto-main's TestingConnectorSession

xumingming · 2021-06-01T14:14:15Z

Sorry for my delay. Really appreciate your contribution! Just a few coding style issues. (I might more focus on the coding style in the first round review. ).

This is really a big one, I will continue the review by the end of this week. Again, thank you for the contribution! Feel free to ping me via slack or reply the comments here if you have any questions or concern.

@beinan Thanks for the review, I will make the changes accordingly this week.

ChunxuTang

@xumingming Nice work! Just added a few comments.
BTW, could you create a doc for this new connector?

ChunxuTang · 2021-06-02T17:58:40Z

...-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreAbstractRecordCursor.java

+    protected abstract void next();
+
+    /**
+     * 为了出错时好定位问题行


Noticed that there're a few other comments in Chinese. Could you remove them or translate them into English?

ChunxuTang · 2021-06-02T18:11:05Z

...-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreAbstractRecordCursor.java

+    public long getReadTimeNanos()
+    {
+        checkClosed();
+        return (currentTimeMillis() - startTimestamp) * 1000_000;


The value of 1000_000 is used multiple times in this file. May we refactor it into a constant variable?

ChunxuTang · 2021-06-02T18:18:30Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreConnectorFactory.java

+            return injector.getInstance(TablestoreConnector.class);
+        }
+        catch (Exception e) {
+            throw Throwables.propagate(e);


nit: What about just throwing an exception here? A quick search of the Presto codebase shows that we don't use Throwables.propagate.

ChunxuTang · 2021-06-02T18:28:24Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreRecordSetProvider.java

+public class TablestoreRecordSetProvider
+        implements ConnectorRecordSetProvider
+{
+    private static final Logger LOG = Logger.get(TablestoreRecordSetProvider.class);


May we use the lower case of LOG here?

ChunxuTang · 2021-06-02T18:29:39Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreRecordSink.java

+public class TablestoreRecordSink
+        implements RecordSink
+{
+    private static final Logger LOG = Logger.get(TablestoreRecordSink.class);


Same here. May we use log?

beinan · 2021-06-01T20:05:36Z

presto-tablestore/pom.xml

+                    </ignoredDependencies>
+                </configuration>
+            </plugin>
+		</plugins>


indentation looks not correct.

beinan · 2021-06-01T20:11:05Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/CustomRetryStrategy.java

+
+import java.util.concurrent.TimeUnit;
+
+public class CustomRetryStrategy


shall we have more meaningful name here? your call

how about timeoutRetryStrategy?

beinan · 2021-06-01T20:13:11Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/IndexFirstSet.java

+    static IndexFirstSet custom(@Nonnull Set<SchemaTableName> tables)
+    {
+        if (tables.size() == 0) {
+            return none();


why not just return NONE? any benefits to use a separate method none()?

seems only be consist with cutom() method, so all place will use these methods rather than direct referencing these fields.

beinan · 2021-06-03T17:37:40Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreValueUtil.java

+            String x = String.valueOf(value);
+            String str = "convert() failed for " + (toPrimaryKeyNotColumnValue ? "PrimaryKeyValue" : "ColumnValue") +
+                    ", required type is " + type + ", and type of value is " + value.getClass() + ", and value=";
+            log.error(str + x, e); //全部打印出来


remove the comments

beinan · 2021-06-03T17:38:16Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreValueUtil.java

+        }
+        catch (Throwable e) {
+            String x = String.valueOf(value);
+            String str = "convert() failed for " + (toPrimaryKeyNotColumnValue ? "PrimaryKeyValue" : "ColumnValue") +


is it better to use String.format? your call
Also we might need a more meaningful name instead of str

beinan · 2021-06-03T17:38:28Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreValueUtil.java

+            if (x.length() > 20) {
+                y = x.substring(0, 20) + "......";
+            }
+            throw new IllegalArgumentException(str + y, e); //打印部分


remove the comment

beinan · 2021-06-03T17:41:22Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreValueUtil.java

+                acceptedTypes = new Class[] {String.class, byte[].class, Slice.class};
+            }
+        }
+        catch (Throwable e) {


Should we handle all the throwable here? I think most of Errors should not handled by application code. What do you think?

Agree with you. Changed to Exception

beinan · 2021-06-03T17:42:45Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreValueUtil.java

+
+            String y = x;
+            if (x.length() > 20) {
+                y = x.substring(0, 20) + "......";


nit: to be consistent, is it better to use triple dot "..."?

beinan · 2021-06-03T17:44:05Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreValueUtil.java

+                String v2 = target.asString();
+                return PrimaryKeyValue.fromString(v2 + "\0");
+            case BINARY:
+                byte[] v3 = target.asBinary();


I don't think v1, v2 and v3 are very meaningful name here.

I agree with you, changed it.

beinan · 2021-06-04T23:29:10Z

@xumingming Nice work! Just added a few comments.
BTW, could you create a doc for this new connector?

Good call, doc is a new requirement to commit guideline https://lists.prestodb.io/g/presto-tsc/topic/adding_doc_requirement_to/81082141?p=,,,20,0,0,0::recentpostdate/sticky,,,20,2,0,81082141

xumingming · 2021-06-27T03:32:29Z

I am back, and will make the necessary changes :)

xumingming · 2021-06-27T12:30:04Z

I have made changes according to your comments, can you help to do another round of review @beinan @ChunxuTang

xumingming · 2021-06-27T12:30:29Z

@xumingming Nice work! Just added a few comments.
BTW, could you create a doc for this new connector?

Good call, doc is a new requirement to commit guideline https://lists.prestodb.io/g/presto-tsc/topic/adding_doc_requirement_to/81082141?p=,,,20,0,0,0::recentpostdate/sticky,,,20,2,0,81082141

Added the documentation.

ChunxuTang

@xumingming Thanks for your huge work! Generally looks good to me. Just left some minor suggestions in the code. @beinan Could you also have another round of review?

BTW, I noticed that there are quite a number of commits in this PR and most of them are just for fixing comments or updating versions. @xumingming When you complete changes, could you merge them into one commit? Thanks~

ChunxuTang · 2021-07-05T17:04:24Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreConfig.java

+    private int clientRetrySeconds = 120;
+    private int connectTimeoutSeconds = 10;
+    private int socketTimeoutSeconds = 50;
+    private int syncClientWaitTimeoutSeconds = 60;


Are these settings configurable by users? If yes, could you add documentation of these settings in the alibaba cloud tablestore doc?

yes, they are configurable, but I'd prefer merge this first PR into prestodb first, and later I can expose more settings for user to tune.

ChunxuTang · 2021-07-05T17:08:23Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreFacade.java

+                .indexName(indexName)
+                .scanQuery(ScanQuery.newBuilder()
+                        .query(buildQueryForCurrentConditions(tupleDomain))
+                        .limit(2000)


Just curious why the value here is 2000. Is it from the empirical experience?

It is recommended by the tablestore team, it is not limiting the result count to 2000, it is limiting to only return 2000 for every request.

ChunxuTang · 2021-07-05T17:12:38Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreMetadata.java

+        List<SchemaTableName> list = listTables(session, prefix.getSchemaName());
+        for (SchemaTableName stn : list) {
+            String qtn = prefix.getTableName();
+            if (qtn != null && !qtn.equalsIgnoreCase(stn.getTableName())) { //如果tableName==null，就匹配全部；否则按tableName过滤


Could you remove the comment or translate it into English?

ChunxuTang · 2021-07-05T17:13:56Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestorePlugin.java

+
+    private static ClassLoader getClassLoader()
+    {
+        ClassLoader tcl = Thread.currentThread().getContextClassLoader(); //PrestoServer对应启动线程的类加载器


Could you remove the comment?

ChunxuTang · 2021-07-05T17:18:58Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreTransactionHandle.java

+        return "TablestoreTransactionHandle{" +
+                "uuid=" + uuid +
+                '}';


Looks that this is not that wide to split into 3 lines. But either way works for me. Your call.

ChunxuTang · 2021-07-05T17:23:31Z

presto-tablestore/src/test/java/com/facebook/presto/tablestore/TestTablestoreFacade.java

+    }
+
+    /**
+     * 类似于：0:(min,0) 1:(0,1) 2:(1,2) 3:(2,3) 4:(3,4) 5:(4,5) 6:(5,6) 7:(6,7) 8:(7,8) 9:(8,max)，第一个是分区，第二个是min，第三个是max


If the comment here is necessary, could you translate it into English?

ChunxuTang · 2021-07-05T17:23:57Z

presto-tablestore/src/test/java/com/facebook/presto/tablestore/TestTablestoreFacade.java

+        List<Split> rList = divideAndMergeAndShuffle(4, list);
+        assertEquals("0:(min,3) 1:(3,5) 2:(5,7) 2:(7,max)", print(rList));
+
+        //希望合并到3份，但也只能合并到4份，因为有些合并不了了


Maybe remove the comment here? I guess no matter we merge them to 3 or 4, it won't influence the major features.

ChunxuTang · 2021-07-05T17:25:00Z

presto-tablestore/src/test/java/com/facebook/presto/tablestore/TestTablestoreFacade.java

+            long n2 = System.currentTimeMillis();
+
+            assertEquals(10000, rList.size());
+            System.out.println("测试性能-" + i + ": " + (n2 - n1) + "ms");


Maybe change the words to English?

ChunxuTang · 2021-07-05T17:26:43Z

...tablestore/src/test/java/com/facebook/presto/tablestore/TestTablestoreSessionProperties.java

+
+        ConnectorSession cs = session();
+        int x = getQueryVersion(cs);
+        assertEquals(2, x); //默认不开启


Maybe remove the comment?

ChunxuTang · 2021-07-05T17:27:00Z

...tablestore/src/test/java/com/facebook/presto/tablestore/TestTablestoreSessionProperties.java

+
+        TestingConnectorSession cs = session();
+        boolean x = enablePartitionPruning(cs);
+        assertTrue(x); //默认开启


Same as above.

xumingming · 2021-07-08T03:04:28Z

@ChunxuTang updated the code according to all the comments, and merged all the relative commits into one commit.

beinan

@ChunxuTang @zhenxiao do you two wanna take a second look? otherwise I will merge this PR

ChunxuTang

LGTM!

xumingming · 2021-07-13T03:03:47Z

Let's merge it?

zhenxiao

nice work, @xumingming
some minor coding style issues

presto-docs/src/main/sphinx/connector/tablestore.rst

presto-tablestore/src/main/java/com/facebook/presto/tablestore/TablestoreFacade.java

xumingming · 2021-07-13T07:19:49Z

@zhenxiao For the return null in RowSearchIterator4CountStar.java, it can return null because the returned value will not be used, it is an optimization for count(*).

xumingming · 2021-07-22T03:52:10Z

@zhenxiao Any further comments?

xumingming · 2021-07-27T05:04:16Z

@zhenxiao Any further comments?

xumingming · 2021-07-28T02:52:47Z

@zhenxiao Any further comments?

xumingming · 2021-07-28T12:21:03Z

@beinan @zhenxiao @ChunxuTang Any further comments?

xumingming · 2021-08-02T03:31:06Z

@zhenxiao Any further comments?

xumingming · 2021-08-03T07:24:23Z

@zhenxiao Any comments?

zhenxiao

thanks, @xumingming
a few more comments

presto-docs/src/main/sphinx/connector/tablestore.rst

presto-tablestore/src/main/java/com/facebook/presto/tablestore/IndexSelectionStrategy.java

zhenxiao · 2021-08-03T13:13:52Z

presto-tablestore/src/main/java/com/facebook/presto/tablestore/ReflectionUtils.java

+            }
+            searchType = searchType.getSuperclass();
+        }
+        return null;


+1. let's make it Optional, instead of returning null

presto-tablestore/src/main/java/com/facebook/presto/tablestore/ReflectionUtils.java

xumingming · 2021-08-09T08:00:40Z

@zhenxiao all comments resolved, can you help review again?

xumingming · 2021-08-27T03:52:59Z

@beinan @zhenxiao @ChunxuTang Any chance to boost the PR review process a little bit? It has already taken 3 months, quite frustrating.

rohanpednekar requested a review from beinan May 27, 2021 00:26

beinan requested changes Jun 1, 2021

View reviewed changes

beinan reviewed Jun 1, 2021

View reviewed changes

ChunxuTang reviewed Jun 2, 2021

View reviewed changes

beinan reviewed Jun 3, 2021

View reviewed changes

xumingming force-pushed the support-alibaba-cloud-tablestore branch from 328d87c to 8f544a3 Compare June 27, 2021 12:23

ChunxuTang reviewed Jul 5, 2021

View reviewed changes

xumingming force-pushed the support-alibaba-cloud-tablestore branch 3 times, most recently from 39071a7 to f88c0f8 Compare July 8, 2021 02:45

xumingming requested a review from beinan July 8, 2021 04:58

beinan approved these changes Jul 8, 2021

View reviewed changes

ChunxuTang approved these changes Jul 8, 2021

View reviewed changes

zhenxiao requested changes Jul 13, 2021

View reviewed changes

xumingming requested a review from zhenxiao July 13, 2021 07:20

xumingming force-pushed the support-alibaba-cloud-tablestore branch from 8b58891 to 1ec7804 Compare July 13, 2021 07:26

xumingming force-pushed the support-alibaba-cloud-tablestore branch from 1ec7804 to 72cfc23 Compare July 22, 2021 04:47

zhenxiao requested changes Aug 3, 2021

View reviewed changes

xumingming added 2 commits August 9, 2021 14:15

Add Alibaba Cloud Tablestore connector

642ed13

Update according to review comments

3a1a676

xumingming force-pushed the support-alibaba-cloud-tablestore branch from 72cfc23 to 3a1a676 Compare August 9, 2021 07:57

xumingming requested a review from zhenxiao August 11, 2021 08:13

xumingming closed this Nov 29, 2021

xumingming deleted the support-alibaba-cloud-tablestore branch November 29, 2021 11:58


		import java.util.concurrent.TimeUnit;

		public class CustomRetryStrategy

Support alibaba cloud tablestore #16151

Support alibaba cloud tablestore #16151

Conversation

xumingming commented May 25, 2021

beinan 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

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

xumingming commented Jun 1, 2021

ChunxuTang left a comment

Choose a reason for hiding this comment

ChunxuTang Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

beinan commented Jun 4, 2021

xumingming commented Jun 27, 2021

xumingming commented Jun 27, 2021

xumingming commented Jun 27, 2021

ChunxuTang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChunxuTang Jun 2, 2021 •

edited

Loading