-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: avoid pushing invalid addr args #195
Conversation
Signed-off-by: Ruihang Xia <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #195 +/- ##
========================================
Coverage 37.76% 37.76%
========================================
Files 16 16
Lines 1300 1300
========================================
Hits 491 491
Misses 704 704
Partials 105 105 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps also for generateMetaSrvAddr
and generateDatanodeAddr
The e2e test failure seems unrelated to this change. It's caused by the partition rule change from DB. We shall fix it in a separate PR |
Signed-off-by: Ruihang Xia <[email protected]>
@waynexia CI failed to your recent PARTITION syntax change - GreptimeTeam/greptimedb#3347 Perhaps you can fix it also? |
Said now this sql is invalid: createTableSQL = `CREATE TABLE dist_table (
ts TIMESTAMP DEFAULT current_timestamp(),
n INT,
row_id INT,
TIME INDEX (ts),
PRIMARY KEY(n)
)
PARTITION BY RANGE COLUMNS (n) (
PARTITION r0 VALUES LESS THAN (5),
PARTITION r1 VALUES LESS THAN (9),
PARTITION r2 VALUES LESS THAN (MAXVALUE),
)` |
Or let's fix the e2e first in another PR first, then we can merge this PR ensuring the tests passed. |
That seems unrelated to this change. Maybe we should pin the db version in the e2e test to also prevent future breaking changes. As those CI are not run in the main repo against every new commit. |
Make sense. Anyway let's avoid merging patch without CI green if possible. (The failed test is unrelated, but it terminates following tests I guess?) |
For this specific case, SQL is run after the cluster is up. |
@waynexia Are you fixing the failure of e2e test? If not, let me fix it in a sperate PR. |
Signed-off-by: Ruihang Xia <[email protected]> Co-authored-by: tison <[email protected]>
closes #194
The generated command now becomes
without invalid args