Skip to content
This repository was archived by the owner on Jul 3, 2023. It is now read-only.

Conversation

@gz
Copy link
Contributor

@gz gz commented May 1, 2023

  • pg-embed improvement for Refactor PgEmbed usage #384 (drop pg-embed instance)
  • UI edit connector fix
  • UI edit pipeline fix
  • UI delete attached connector fix
  • UI pipeline stats not getting displayed.

gz added 2 commits May 1, 2023 13:46
Turns out if it's in a once-cell drop doesn't get called if the
developer hits ctrl+c which means next time the manager is restarted
it won't start again. So we put it back in ProjectDb.
Also removed oncecell dependency which means we can simplify feature
to just 'pg-embed'.
@ryzhyk
Copy link
Collaborator

ryzhyk commented May 1, 2023

There was also the issue with pipeline stats not getting displayed.

gz added 3 commits May 1, 2023 17:28
If we do it anyways we'll potentially overwrite the changes
done by the user if they happen in parallel.
We reset the form after a connector is edited/added
and switch back to the initial tab.
We also update the form with the current connector for edit
by using useEffect which guarantees it will re-render with
the correct form values.
pipeline stats werent displyaed anymore because endpoint_name
semantics has changed.
@gz gz temporarily deployed to github-pages May 2, 2023 05:55 — with GitHub Actions Inactive
@gz gz temporarily deployed to github-pages May 2, 2023 05:58 — with GitHub Actions Inactive
@gz gz marked this pull request as ready for review May 2, 2023 06:12
@gz gz requested a review from ryzhyk May 2, 2023 06:13
gz added 2 commits May 1, 2023 23:47
Disallow backend fetches to override local state
that's been modified.
Another PR changed how we named endpoints.
@gz gz temporarily deployed to github-pages May 2, 2023 06:51 — with GitHub Actions Inactive
@gz gz temporarily deployed to github-pages May 2, 2023 06:54 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #389 (1ba6712) into main (34ea1f3) will decrease coverage by 0.03%.
The diff coverage is 61.11%.

❗ Current head 1ba6712 differs from pull request most recent head 4fb7320. Consider uploading reports for the commit 4fb7320 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
- Coverage   73.85%   73.82%   -0.03%     
==========================================
  Files         238      238              
  Lines       51586    51581       -5     
==========================================
- Hits        38097    38081      -16     
- Misses      13489    13500      +11     
Impacted Files Coverage Δ
crates/dataflow-jit/src/ir/validate.rs 30.01% <ø> (+0.85%) ⬆️
crates/pipeline_manager/src/config.rs 0.00% <ø> (ø)
crates/pipeline_manager/src/db/mod.rs 86.34% <55.17%> (-3.00%) ⬇️
crates/pipeline_manager/src/db/test.rs 92.19% <85.71%> (-0.81%) ⬇️

... and 2 files with indirect coverage changes

@github-actions
Copy link

github-actions bot commented May 2, 2023

Benchmark results

Nexmark

  • Compared results from dbe4663 (main) with 1ba6712 (PR)
    No benchmark results found for current main revision, compared against dbe4663
name main~57 [kOp/s] PR [kOp/s] Tput change [%] Assessment Peak RSS diff
q0 5262.63 5436.07 3 ✔️ -11.3 MB
q1 5429.6 5226.95 -4 ✔️ -4.0 MB
q2 5361.6 5437.65 1 ✔️ -3.7 MB
q3 5321.94 5447.48 2 ✔️ 16.1 MB
q4 4802.55 4786.13 0 ✔️ -69.5 MB
q5 5378.33 5338.66 -1 ✔️ -69.5 MB
q6 4880.13 4909.42 1 ✔️ -242.4 MB
q7 2559.31 4097.32 60 🌲 -5.8 GB
q8 5161.2 5088.91 -1 ✔️ -5.8 GB
q9 875.448 871.473 0 ✔️ -268.7 MB
q12 4910.28 4855.35 -1 ✔️ -268.7 MB
q13 3645.17 3560.01 -2 ✔️ -268.7 MB
q14 5481.43 5368.21 -2 ✔️ -268.7 MB
q15 4895.11 5266.21 8 🌲 -268.7 MB
q16 1045.16 1046.67 0 ✔️ -268.7 MB
q17 3134.56 3086.27 -2 ✔️ -75.1 MB
q18 1440.13 1439.51 0 ✔️ 788.6 MB
q19 1435.15 1441.03 0 ✔️ 788.6 MB
q20 1500.83 1574.52 5 ✔️ 788.6 MB
q21 5117.36 5319.12 4 ✔️ 788.6 MB
q22 5356.59 5421.79 1 ✔️ 788.6 MB

Galen

  • Compared results from dbe4663 (main) with 1ba6712 (PR)
    No benchmark results found for current main revision, compared against dbe4663
name main~57 [s] PR [s] Runtime change [%] Assessment
galen 28.6952 28.966 1 ✔️

LDBC

  • 1 out of 4 queries have regressed ❗
  • Compared results from dbe4663 (main) with 1ba6712 (PR)
    No benchmark results found for current main revision, compared against dbe4663
algorithm dataset threads main~57 [kEVPS] PR [kEVPS] Tput change [%] Assessment Peak RSS diff
bfs graph500-22 1 1835.7 1786.87 -3 ✔️ 20.5 kB
bfs datagen-8_4-fb 6 8173.61 7584.51 -7 🔻 28.0 MB
pagerank graph500-22 1 681.655 687.644 1 ✔️ -69.6 kB
pagerank datagen-8_4-fb 6 1994.79 2020.01 1 ✔️ -9.4 MB

Nexmark (with Persistence)

  • 1 out of 21 queries have regressed ❗
  • Compared results from dbe4663 (main) with 1ba6712 (PR)
    No benchmark results found for current main revision, compared against dbe4663
name main~57 [kOp/s] PR [kOp/s] Tput change [%] PR DRAM [kOp/s] DRAM diff [%] Assessment
q0 2399.23 2488.04 4 2343.28 6 ✔️
q1 1713.93 1707.41 0 1651.32 3 ✔️
q2 2407.67 2425.31 1 2351.95 3 ✔️
q3 2036.93 2060.41 1 2255.63 -9 ✔️
q4 360.256 363.319 1 1401.39 -74 ✔️
q5 2015.3 2053.24 2 2051.77 0 ✔️
q6 332.421 335.283 1 1371.28 -76 ✔️
q7 639.584 664.104 4 1302.55 -49 ✔️
q8 2171.19 2272.97 5 2224.2 2 ✔️
q9 79.0764 80.0189 1 379.802 -79 ✔️
q12 838.481 870.68 4 1785.64 -51 ✔️
q13 443.491 444.844 0 988.451 -55 ✔️
q14 1697.52 1706.38 1 1703.05 0 ✔️
q15 198.112 198.86 0 1189.09 -83 ✔️
q16 25.5771 25.7377 1 284.756 -91 ✔️
q17 81.4521 81.1497 0 785.149 -90 ✔️
q18 124.763 124.774 0 790.672 -84 ✔️
q19 182.489 181.242 -1 647.996 -72 ✔️
q20 505.761 507.754 0 932.781 -46 ✔️
q21 1489.8 1520.22 2 1488.73 2 ✔️
q22 2102.89 1959.12 -7 2052.72 -5 🔻

// lifetime of the program.
#[cfg(feature = "pg-embed")]
#[allow(dead_code)] // It has to stay alive until ProjectDB is dropped.
pg_inst: Option<pg_embed::postgres::PgEmbed>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I understand, how is this more robust to dropping during kill-9 than a shared reference with OneCell?

Copy link
Contributor Author

@gz gz May 2, 2023

Choose a reason for hiding this comment

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

If we press ctrl+c on the manager in the console running as dev, and pg_inst is in a OnceCell, then drop() is not called, and so the Postgres process remains running (pg-embed does not stop it). So if you restart it after, it complains because it can't bring up the postgres instance again (it's still running the stale instance). So you have to manually go and kill postgres which is a bit annoying when working on the manager. If PgEmbed is in the struct then drop() gets called and Postgres is stopped.

with kill-9 this is still a problem even now: the postgres daemon is not cleaned up and we have to do it manually. But hopefully we won't need to kill-9 the process a lot anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that, but what I don't get is:

If OnceCell is dropped the PgEmbed instance should also be dropped right? Same as when we add the PgEmbed instance to the ProjectDB struct? What makes OnceCell not be dropped during sigterm?

Copy link
Contributor Author

@gz gz May 3, 2023

Choose a reason for hiding this comment

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

Because it's a static:

Static items do not call drop at the end of the program.
https://doc.rust-lang.org/reference/items/static-items.html
https://users.rust-lang.org/t/about-drop-call-of-static-object/50544/4

It has been discussed as expected behavior for OnceCell: matklad/once_cell#98
e.g., link suggest to use atexit functionality with other libraries

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, missed the part about it being static. Thanks.

If we don't update the query cache the UI will re-create
the just deleted from the query cache one savestatus goes to
isUptoDate.
@gz gz temporarily deployed to github-pages May 2, 2023 16:15 — with GitHub Actions Inactive
@gz gz temporarily deployed to github-pages May 2, 2023 16:18 — with GitHub Actions Inactive
@ryzhyk ryzhyk merged commit d696c74 into main May 2, 2023
@ryzhyk ryzhyk deleted the ui-regres branch May 2, 2023 16:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants