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

Optimize BGW CI tests #7223

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Optimize BGW CI tests #7223

merged 1 commit into from
Sep 4, 2024

Conversation

kpan2034
Copy link
Contributor

This change helps optimize two bgw tests, by a) reducing the time a test spends waiting for scheduled jobs and b) decreasing the amount of data inserted into the table. This should have no effect on the efficacy of the tests themselves.

Disable-check: force-changelog-file

This change helps optimize two bgw tests, by a) reducing the time a
test spends waiting for scheduled jobs and b) decreasing the amount of
data inserted into the table. This should have no effect on the efficacy
of the tests themselves.
@kpan2034 kpan2034 added the ci label Aug 29, 2024
@kpan2034 kpan2034 self-assigned this Aug 29, 2024
@kpan2034
Copy link
Contributor Author

These are the improvements I saw when running this locally:

Test Before After
bgw_custom 46.7s 29.4s
bgw_job_stat_history_errors 35.4s 15.4s

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.78%. Comparing base (59f50f2) to head (9b3c9ed).
Report is 308 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7223      +/-   ##
==========================================
+ Coverage   80.06%   81.78%   +1.71%     
==========================================
  Files         190      205      +15     
  Lines       37181    38320    +1139     
  Branches     9450     9936     +486     
==========================================
+ Hits        29770    31341    +1571     
+ Misses       2997     2987      -10     
+ Partials     4414     3992     -422     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

COMMIT;
END
$$;
CREATE OR REPLACE PROCEDURE custom_proc2(jobid int, config jsonb) LANGUAGE PLPGSQL AS
$$
BEGIN
UPDATE custom_log set msg = 'msg2' where msg = 'msg0';
perform pg_sleep(10);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like custom_proc2 waits for anything in this test so I eliminated this sleep call entirely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is actually such kind of test should be an isolation test instead. This sleep is to make sure we'll end up with a concurrent update issue: https://github.com/timescale/timescaledb/blob/main/tsl/test/expected/bgw_job_stat_history_errors.out#L96

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still see the same error -- I don't see why custom_proc2 needs to sleep for that to happen. Doesn't the concurrent update issue occur immediately when custom_proc2 tries to update a row that custom_proc1 already updated?

I agree that ideally this should be an isolation test.

@@ -549,7 +549,7 @@ INSERT INTO sensor_data
random() AS cpu,
random()* 100 AS temperature
FROM
generate_series(:'start_date_sd'::timestamptz - INTERVAL '1 months', :'start_date_sd'::timestamptz - INTERVAL '1 week', INTERVAL '1 minute') AS g1(time),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These inserts were taking a significant amount of time, so I reduced the amount of data being generated. I don't think this affects test behavior itself.

Copy link
Contributor

@antekresic antekresic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with doing this as long as it doesn't affect the flakiness of the test i.e. makes it more prone to failure.

Can you run the CI here multiple times to ensure that isn't happening? I say this because bgw_custom is notorious for being flaky and we don't want to make the situation worse here.

@kpan2034 kpan2034 merged commit 8a6c8cd into timescale:main Sep 4, 2024
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants