Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tck testcases strengthening #2797

Conversation

bright-starry-sky
Copy link
Contributor

Remove unnecessary index rebuilding.
The index has been created before the data import, so instead of rebuilding.
Let's verify the stability of CI.

@bright-starry-sky bright-starry-sky added the ready-for-testing PR: ready for the CI test label Sep 6, 2021
@bright-starry-sky
Copy link
Contributor Author

build failed of ubuntu2004, gcc-9.2
[ 81%] Building CXX object src/meta/CMakeFiles/meta_service_handler.dir/ActiveHostsMan.cpp.o [ 81%] Building CXX object src/meta/CMakeFiles/meta_service_handler.dir/MetaServiceUtils.cpp.o [ 81%] Building CXX object src/meta/CMakeFiles/meta_service_handler.dir/processors/parts/ListPartsProcessor.cpp.o [ 81%] Building CXX object src/meta/CMakeFiles/meta_service_handler.dir/processors/parts/ListHostsProcessor.cpp.o [ 81%] Building CXX object src/meta/CMakeFiles/meta_service_handler.dir/processors/parts/CreateSpaceProcessor.cpp.o Error: Process completed with exit code 137.

@bright-starry-sky bright-starry-sky force-pushed the tck_testcase_strengthening branch from 5dfb10d to eed0bbe Compare September 6, 2021 13:29
@yixinglu
Copy link
Contributor

yixinglu commented Sep 6, 2021

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the check is based on the job status instead of directly waiting for a fixed time.

And we still have many non-default index scenarios that need to be rebuild. This step is still useful.

@bright-starry-sky
Copy link
Contributor Author

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the checks is based on the job status instead of directly waiting for a fixed time

I see this wait only applies to the data import phase, just wait for the rebuild job, which is unnecessary. right?

@bright-starry-sky
Copy link
Contributor Author

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the check is based on the job status instead of directly waiting for a fixed time.

And we still have many non-default index scenarios that need to be rebuild. This step is still useful.

@yixinglu
Copy link
Contributor

yixinglu commented Sep 6, 2021

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the checks is based on the job status instead of directly waiting for a fixed time

I see this wait only applies to the data import phase, just wait for the rebuild job, which is unnecessary. right?

I just don’t want to use sleep to check the job status, because sleep is sometimes unreliable

@bright-starry-sky
Copy link
Contributor Author

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the checks is based on the job status instead of directly waiting for a fixed time

I see this wait only applies to the data import phase, just wait for the rebuild job, which is unnecessary. right?

I just don’t want to use sleep to check the job status, because sleep is sometimes unreliable

In this PR, The sleep is not for the state of the job, it is just waiting for the MetaClient, same as sleep(heartbeat_interval_secs)

@critical27
Copy link
Contributor

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the checks is based on the job status instead of directly waiting for a fixed time

I see this wait only applies to the data import phase, just wait for the rebuild job, which is unnecessary. right?

I just don’t want to use sleep to check the job status, because sleep is sometimes unreliable

Why not just insert data with index? We don't use rebuild for all cases.

@yixinglu
Copy link
Contributor

yixinglu commented Sep 7, 2021

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the checks is based on the job status instead of directly waiting for a fixed time

I see this wait only applies to the data import phase, just wait for the rebuild job, which is unnecessary. right?

I just don’t want to use sleep to check the job status, because sleep is sometimes unreliable

Why not just insert data with index? We don't use rebuild for all cases.

This is true for scenarios that are just query nebula, but there are also some DDL/DML scenarios

@yixinglu
Copy link
Contributor

yixinglu commented Sep 7, 2021

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the checks is based on the job status instead of directly waiting for a fixed time

I see this wait only applies to the data import phase, just wait for the rebuild job, which is unnecessary. right?

I just don’t want to use sleep to check the job status, because sleep is sometimes unreliable

In this PR, The sleep is not for the state of the job, it is just waiting for the MetaClient, same as sleep(heartbeat_interval_secs)

Even so, i also don't agree to delete all wait job related steps. at present, it's just for rebuild index, but there are some other jobs we could test them by these steps. And i prefer to use these steps in lookup test cases to replace the wait x seconds steps, but i could not do that since the cause of cache updating.

if you don't want to wait in import data, you can only delete the line of waiting job finished. please don't delete these wait implementations.

@critical27
Copy link
Contributor

Disagree to delete all wait steps. If the index has been built, wait will not consume much time, because the checks is based on the job status instead of directly waiting for a fixed time

I see this wait only applies to the data import phase, just wait for the rebuild job, which is unnecessary. right?

I just don’t want to use sleep to check the job status, because sleep is sometimes unreliable

Why not just insert data with index? We don't use rebuild for all cases.

This is true for scenarios that are just query nebula, but there are also some DDL/DML scenarios

If the problem is to reduce wait time, still could remove the REBUILD INDEX even when DDL/DML. Would you explain why we need to REBUILD for each case?

@HarrisChu
Copy link
Contributor

how about just delete code for import?
https://github.com/vesoft-inc/nebula/blob/master/tests/tck/conftest.py#L115 and https://github.com/vesoft-inc/nebula/blob/master/tests/tck/conftest.py#L174

image

@bright-starry-sky bright-starry-sky force-pushed the tck_testcase_strengthening branch from eed0bbe to 96afe8b Compare September 8, 2021 01:55
@bright-starry-sky
Copy link
Contributor Author

I don't think wait_index_ready makes any sense , If you need the rebuild index in a test case, you should call the rebuild command instead of this macro. By the way, keep this method, but it's not what I want.

@bright-starry-sky bright-starry-sky merged commit f7e60d3 into vesoft-inc:master Sep 8, 2021
@bright-starry-sky bright-starry-sky deleted the tck_testcase_strengthening branch September 8, 2021 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants