-
Notifications
You must be signed in to change notification settings - Fork 2
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
Autosharding tests: inputs & syntax validation #158
Comments
The syntax for sharding is
The tests should cover the following cases and ensure that we have implemented the syntax correctly. They should return an error when required, and also perform the logic correctly when there are no errors:
Should there be additional cases to examine within the syntax/validation group, we ought to include those as well. |
Colleagues, spent a lot of time, tried different ways. However, tests with Logs:
|
Can I have original |
Here is my file |
The solution was discussed in Slack - https://manticoresearch.slack.com/archives/C06270TRJAD/p1697711746155199 : Here's how you can start Manticore to prepare autosharding tests locally:
|
The purpose of this task is to test the syntax. Whether a table is create or not is covered in another test - #159 @PavelShilin89 pls ask @donhardman to review your PR once the tests are done. |
@PavelShilin89 What are waiting for from @donhardman ? I see you've assigned this issue to him, but didn't provide any comment. |
Which pull request is related to this task? |
@donhardman all the changes were in https://github.com/manticoresoftware/manticoresearch/pull/1595/files. I'm already looking at your comments on the pull request. |
Next, I made a new branch and a new pull request (https://github.com/manticoresoftware/manticoresearch/pull/1642/files) https://github.com/manticoresoftware/manticoresearch/pull/1642/files. To separate our tasks. |
@PavelShilin89 it's not clear what each PR is about: Please update the titles. |
I removed the |
@donhardman Please check the tests in the
Please give feedback and guide whether the behavior and error notifications are as desired. |
As we discussed with Sergey, we'll stick to the plan where both shards and rf need to be integers. So, the correct error has been addressed as expected behavior, and it's not being sent to the Buddy at all.
This is expected, behaviour, please update tests
Also expected, please update, valid query no error is valid:
This one also valid, cuz we just proxy now the query to the daemon and daemon ignores options that cannot handle, update this case:
This looks weird for me, I will check and fix in case we have issue:
|
I have fixed shards=a rf=b, please validate other fixes |
@donhardman I noticed that in the test |
@donhardman In the
|
Here's the situation: The problem starts when a daemon syncs the tables to Server #1 and overwrites everything there. Consequently, the buddy daemon, already loaded with the information that it's the master node (since it was set up as local), fails to recognize the cluster setup. Therefore, we can't detect this issue unless we run a consistency check, which isn't ideal. The process here seems flawed. We shouldn't be testing this, or we need to implement a way to validate consistency changes on the buddy daemon—which isn't a great solution either. Alternatively, we could simply throw an error when attempting actions that shouldn't be executed in the first place. |
To simplify:
causes the issue. We are not going to make improvements to support it for now (as it requires significant changes including in the architecture). We'd better just write to the docs about this edge case. @PavelShilin89 please update the test accordingly. @donhardman pls update it in the docs. |
@donhardman as discussed, pls also explain to Pavel the difference between the syntax and functionality tests that were designed initially. |
As we discussed earlier, we decided it's a good idea to align the tests with the original highlight: inputs and syntax validation. This means we already have tests covering replication logic and cluster creation, so this task is mainly about syntax. What we should check is just the syntax, ensuring it works as expected without generating any results. Additionally, in the case where we create a local table and then try to create a clustered table on another node – this is a current limitation. I advise grouping the tests by requirements in different files for easier understanding. Also, to test the syntax, we don't need to start 8 cluster nodes, so let's save some time by running syntax checks only. We have other checks that perform multi-node tests. If you want to add tests with 8 cluster nodes, feel free to add an extra .rec file there. Currently, we're testing up to 5 nodes. As for a pull request or documentation, we don't have that yet because we're still waiting until all the tests are completed to ensure it works properly. I tried searching for it but couldn't find anything. I've created a task to implement it. |
The text was updated successfully, but these errors were encountered: