You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2021/04/01 03:18:37 UTC
[GitHub] [iotdb] yifuzhou opened a new pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
yifuzhou opened a new pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958
## Description
In current cluster version, if we have more than 2,000 timeseries, when we commend SHOW TIMESERIES, it will only display 2,000 timeseries, the same as the SHOW DEVICES...
In the default settings, if we do not set any limit, the default plan.limit will be 1000(set as the fetchsize). This is weird, because if we do not set the limit, it means that I want to fetch all the data instead of fetchsize. So I remove it.
In my opinion, hasNextWithoutConstraint() method is not correct, after I have already fetched all the timeseries, it is not necessary to do fetch again... For example, if we have 10,000 timeseries, so the result.size()==10,000. After the first display(fetch size==1000), it will only display the first 1000 timeseries, now the index is 1000, so it is still index < result.size() and it will keep display the rest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] LebronAl commented on pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
Posted by GitBox <gi...@apache.org>.
LebronAl commented on pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958#issuecomment-834118775
> Hi all. This PR is still in progress actually. In this PR, first, if we do not set the 'limit', it will fetch as large as it can, it may cause the problem of Memory Explosion. Second, the sequence of fetch data is not in order, I think it violates the purpose of 'offset' and 'limit' field because we can find what we want by SHOW TIMESERIES OFFSET ? LIMIT ?.
> I will fix it later and after the IT for cluster merged, I will add the test case of this part, thanks!
ping, you can add IT tests now~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] qiaojialin commented on pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958#issuecomment-827602246
Hi, to make the offset and limit meaningful in show timeseries, we need to manage timeseries in some order in the MTree firstly.
Besides, I remember that hasNextWithoutConstraint() does not consider the offset and limit. Only hasNext considers the offset and limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jt2594838 commented on a change in pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958#discussion_r605496583
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/metadata/CMManager.java
##########
@@ -1519,7 +1519,6 @@ public MNode getMNode(MNode deviceMNode, String measurementName) {
// do not use limit and offset in sub-queries unless offset is 0, otherwise the results are
// not combinable
if (offset != 0) {
- plan.setLimit(0);
Review comment:
This should not be removed. Consider this case:
The offset is 10000, the limit is 10, and the results are distributed on 3 nodes. If you keep the limit, each of the 3 nodes will only return 10 results, 30 results in total, which will all be filtered when the offset is applied.
You may set the limit to `limit + offset` so the unnecessary fetch can be reduced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958#issuecomment-824169998
Hi, no test... could you add a screenshot to show the correction?
BTW, once PR https://github.com/apache/iotdb/pull/3024 is merged, we can add IT for the cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jt2594838 merged pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
Posted by GitBox <gi...@apache.org>.
jt2594838 merged pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] yifuzhou commented on pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
Posted by GitBox <gi...@apache.org>.
yifuzhou commented on pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958#issuecomment-826450158
Hi all. This PR is still in progress actually. In this PR, first, if we do not set the 'limit', it will fetch as large as it can, it may cause the problem of Memory Explosion. Second, the sequence of fetch data is not in order, I think it violates the purpose of 'offset' and 'limit' field because we can find what we want by SHOW TIMESERIES OFFSET ? LIMIT ?.
I will fix it later and after the IT for cluster merged, I will add the test case of this part, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] yifuzhou commented on pull request #2958: [IOTDB-1266]SHOW TIMESERIES will only display 2000 timeseries
Posted by GitBox <gi...@apache.org>.
yifuzhou commented on pull request #2958:
URL: https://github.com/apache/iotdb/pull/2958#issuecomment-826450158
Hi all. This PR is still in progress actually. In this PR, first, if we do not set the 'limit', it will fetch as large as it can, it may cause the problem of Memory Explosion. Second, the sequence of fetch data is not in order, I think it violates the purpose of 'offset' and 'limit' field because we can find what we want by SHOW TIMESERIES OFFSET ? LIMIT ?.
I will fix it later and after the IT for cluster merged, I will add the test case of this part, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org