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

Add bulk_insert option to add list of records using auditor client and pyauditor #582

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

raghuvar-vijay
Copy link
Collaborator

This closes #580

Bulk insertion of records is implemented for auditor. This allows list of records to be passed as an argument to auditor client and pyauditor methods.
Added unit test, API tests for auditor and pyauditor client.

Created new end point called /records - this will be used to perform bulk operations (in the future, this can be extended to update as well using PUT method)

Written doc strings and tutorial to use bulk_insert functionality.

Added CHANGELOG.md

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2023

Codecov Report

Merging #582 (06ebd00) into main (6641a27) will increase coverage by 0.41%.
The diff coverage is 79.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #582      +/-   ##
==========================================
+ Coverage   64.06%   64.47%   +0.41%     
==========================================
  Files          46       46              
  Lines        5387     5531     +144     
  Branches       58       58              
==========================================
+ Hits         3451     3566     +115     
- Misses       1936     1965      +29     
Components Coverage Δ
apel_plugin 67.47% <ø> (ø)
auditor 86.01% <97.45%> (+0.54%) ⬆️
slurm_collector 65.93% <ø> (ø)
slurm_epilog_collector 0.00% <ø> (ø)
htcondor_collector ∅ <ø> (∅)
priority_plugin 36.00% <ø> (ø)

Copy link
Contributor

@QuantumDancer QuantumDancer left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I only have a few minor comments, mostly related to the documetation, see below.

Comment on lines 115 to 118
//! # let records: Result<Vec<RecordAdd>, _> = (0..5)
//! # .map(|i| RecordAdd::new(&format!("record-{}", i), HashMap::new(), vec![], start_time))
//! # .collect();
//! client.bulk_insert(&records?).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can records be of type Vec<RecordAdd> so that we then have client.bulk_insert(&records).await?; inside the docs?

Comment on lines 185 to 186
#[tracing::instrument(name = "Adding a record to the database", skip(records, pool))]
pub async fn bulk_add(
Copy link
Contributor

Choose a reason for hiding this comment

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

The tracing instrument name doesn't match what the function is doing. Maybe switch to "Adding multiple records to the database" or something like this.

@@ -98,6 +98,14 @@ Assuming that a record and a client were already created, the record can be push

await client.add(record)

Pushing list of records to Auditor
Copy link
Contributor

Choose a reason for hiding this comment

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

Pushing list -> Pushing a list

@@ -121,6 +121,19 @@ impl AuditorClientBlocking {
.map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(format!("{e}")))
}

/// add(record: Record)
/// Push a record to the Auditor instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above (a record != bulk_insert)

@@ -797,6 +797,25 @@ impl AuditorClient {
})
}

/// add(record: Record)
/// Push a record to the Auditor instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@raghuvar-vijay raghuvar-vijay force-pushed the bulk-insert branch 2 times, most recently from 57431ca to 06ebd00 Compare December 7, 2023 10:08
@raghuvar-vijay raghuvar-vijay merged commit 17aad52 into ALU-Schumacher:main Dec 7, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk insert and update of AUDITOR records using auditor client
3 participants