Moved to Axum and async #33

dpp · 2025-02-10T16:30:01Z

💻 Description of Change(s) (w/ context)

Moved From Roullie web framework (synchronous) to Axum (tokio's own web framework and async)

Updated a lot of code to be async. Addressed some async performance issues related to BufReader

Cleaned up routing code

🧠 Rationale Behind Change(s)

Moving to a supported web framework is important.

Axum is OpenAPI-friendly and soon there'll be OpenAPI endpoints

Axum's routing scheme is easier to layer on top of which will be important for other applications
that embed Big Tent.

Async is Rust's future (ha ha)... so might as well get on the Async train... or at least Promise to...

📝 Test Plan

All existing tests work

📜 Documentation

Added code documentation

💣 Quality Control

(All items must be checked before a PR is merged)
Did you…

Mention an issue number in the PR title?
Update the version # in the build file?
Create new and/or update relevant existing tests?
Create or update relevant documentation and/or diagrams?
Comment your code?
Fix any stray verbose logging (removing, or moving to debug / trace level)?

Before Merging…

Make sure the Quality Control boxes are all ticked
Make sure any open comments or conversations on the PR are resolved

…mework Signed-off-by: David Pollak <[email protected]>

Signed-off-by: David Pollak <[email protected]>

…d addressing some performance issues with BufReader Signed-off-by: David Pollak <[email protected]>

Signed-off-by: David Pollak <[email protected]>

aredridel · 2025-02-10T16:44:33Z

src/cluster_writer.rs

+  // 25M
+  const MAX_DATA_FILE_SIZE: u64 = 15 * 1024 * 1024 * 1024;
+  const MAX_INDEX_CNT: usize = 25 * 1024 * 1024;
+
+  // 15GB


Comments got dissociated from their target

src/data_file.rs

aredridel · 2025-02-10T16:51:49Z

src/index_file.rs

-      .file
-      .lock()
-      .map_err(|e| anyhow!("Failed to lock {:?}", e))?;
+    let mut my_file = self.file.lock().await;


Interesting. Locking that's infallible?

Probably causes the whole Tokio runtime to panic/collapse... but that'll never happen in production at 2am... never!

aredridel · 2025-02-10T16:59:45Z

src/server.rs

+    .route("/bulk", post(serve_bulk))
+    .route("/{*gitoid}", get(serve_gitoid))
+    .route("/aa/{*gitoid}", get(serve_anti_alias))
+    .route("/north/{*gitoid}", get(serve_north))
+    .route("/north_purls/{*gitoid}", get(serve_north_purls))
+    .route("/north", post(serve_bulk_north))
+    .route("/north_purls", post(serve_bulk_north_purls))
+    .with_state(state);


So much nicer to parse as a person trying to understand the structure of the app

dpletter · 2025-02-10T18:24:24Z

src/config.rs

-  pub fn num_threads(&self) -> u16 {
-    self.threads.unwrap_or(7)
-  }
+  // pub fn num_threads(&self) -> u16 {


Is this no longer needed? Should it be removed?

dpletter · 2025-02-10T18:25:53Z

src/data_file.rs

-    let mut data_file = GoatRodeoCluster::find_file(dir, hash, "grd")?;
+  pub async fn new(dir: &PathBuf, hash: u64) -> Result<DataFile> {
+    let mut data_file = GoatRodeoCluster::find_file(dir, hash, "grd").await?;
+


what is this magic value "grd" ? should it be a named constant?

"Goat rodeo data"

I'm definitely on team "just inline them please, so I don't have to chase a zillion references to read anything"

Created constants...

dpletter

just left a couple nit-picky questions

Signed-off-by: David Pollak <[email protected]>

aredridel · 2025-02-10T22:16:47Z

src/rodeo_server.rs

-  pub fn get_cluster(&self) -> GoatRodeoCluster {
-    let the_cluster: GoatRodeoCluster = (&(**self.get_cluster_arcswap().load())).clone();
+  pub fn get_cluster(&self) -> Arc<GoatRodeoCluster> {
+    let the_cluster: Arc<GoatRodeoCluster> = (&(*self.get_cluster_arcswap().load())).clone();


This smells funny, I might unpack that later and see if I can figure out why the reference hackery is needed. Looks like it does the right thing though so shipit

It's because we need to pass Arc<GoatRodeoCluster> around... and clone()ing an Arc is much cheaper than cloning the underlying data structure.

okay but the reference and pointer creation isn't needed:

Suggested change

let the_cluster: Arc<GoatRodeoCluster> = (&(*self.get_cluster_arcswap().load())).clone();

let the_cluster: Arc<GoatRodeoCluster> = self.get_cluster_arcswap().load().clone();

src/rodeo_server.rs

aredridel · 2025-02-10T22:22:07Z

src/rodeo_server.rs

+  pub fn get_cluster(&self) -> Arc<GoatRodeoCluster> {
+    let the_cluster: Arc<GoatRodeoCluster> = (&(*self.get_cluster_arcswap().load())).clone();


Or even this:

Suggested change

pub fn get_cluster(&self) -> Arc<GoatRodeoCluster> {

let the_cluster: Arc<GoatRodeoCluster> = (&(*self.get_cluster_arcswap().load())).clone();

pub fn get_cluster(&self) -> Arc<GoatRodeoCluster> {

self.get_cluster_arcswap().load().clone()

Signed-off-by: David Pollak <[email protected]>

dpp and others added 4 commits February 9, 2025 20:44

First bit of refactor of Big Tent to using async and the axum web fra…

b940275

…mework Signed-off-by: David Pollak <[email protected]>

Implemented additional features... should be at parity with old version

f2aa6aa

Signed-off-by: David Pollak <[email protected]>

Addressed an issue with a nightly feature I was using and also starte…

d6b7143

…d addressing some performance issues with BufReader Signed-off-by: David Pollak <[email protected]>

Fixed performance issues using async bufreader. Fixed routing problems

94a81e9

Signed-off-by: David Pollak <[email protected]>

dpp requested review from aredridel, earldouglas and dpletter February 10, 2025 16:30

aredridel approved these changes Feb 10, 2025

View reviewed changes

dpletter reviewed Feb 10, 2025

View reviewed changes

earldouglas approved these changes Feb 10, 2025

View reviewed changes

dpletter reviewed Feb 10, 2025

View reviewed changes

dpp added 2 commits February 10, 2025 17:06

Integrated the flatten API endpoint and functionality

5013723

Signed-off-by: David Pollak <[email protected]>

Cleaned up some stuff based on PR comments/feedback

908e301

Signed-off-by: David Pollak <[email protected]>

dpp requested review from aredridel, earldouglas and dpletter February 10, 2025 22:16

aredridel reviewed Feb 10, 2025

View reviewed changes

aredridel approved these changes Feb 10, 2025

View reviewed changes

aredridel reviewed Feb 10, 2025

View reviewed changes

src/rodeo_server.rs Outdated Show resolved Hide resolved

aredridel reviewed Feb 10, 2025

View reviewed changes

More typo, etc. cleanup

e48bbd2

Signed-off-by: David Pollak <[email protected]>

aredridel approved these changes Feb 10, 2025

View reviewed changes

More cleanup based on Aria's feedback

08faceb

Signed-off-by: David Pollak <[email protected]>

dpp requested a review from aredridel February 10, 2025 22:27

aredridel approved these changes Feb 10, 2025

View reviewed changes

dpp merged commit edff1f6 into main Feb 10, 2025
1 check passed

dpp deleted the poem_version branch February 12, 2025 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved to Axum and async #33

Moved to Axum and async #33

dpp commented Feb 10, 2025

aredridel Feb 10, 2025

dpp Feb 10, 2025

aredridel Feb 10, 2025

dpp Feb 10, 2025

aredridel Feb 10, 2025

aredridel Feb 10, 2025

dpletter Feb 10, 2025

dpp Feb 10, 2025

dpletter Feb 10, 2025

aredridel Feb 10, 2025

dpp Feb 10, 2025

dpletter left a comment

aredridel Feb 10, 2025

dpp Feb 10, 2025

aredridel Feb 10, 2025

aredridel Feb 10, 2025 •

edited

Loading

	let the_cluster: Arc<GoatRodeoCluster> = (&(*self.get_cluster_arcswap().load())).clone();
	let the_cluster: Arc<GoatRodeoCluster> = self.get_cluster_arcswap().load().clone();

		pub fn get_cluster(&self) -> Arc<GoatRodeoCluster> {
		let the_cluster: Arc<GoatRodeoCluster> = (&(*self.get_cluster_arcswap().load())).clone();

Moved to Axum and async #33

Moved to Axum and async #33

Conversation

dpp commented Feb 10, 2025

💻 Description of Change(s) (w/ context)

🧠 Rationale Behind Change(s)

📝 Test Plan

📜 Documentation

💣 Quality Control

Before Merging…

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpletter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aredridel Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

aredridel Feb 10, 2025 •

edited

Loading