Out of band email #67

jbr · 2023-05-02T20:42:56Z

No description provided.

src/queue.rs

src/entity/queue.rs

src/queue.rs

src/clients/auth0_client.rs

src/queue.rs

branlwyd · 2023-05-02T23:45:23Z

src/queue.rs

+            [JobStatus::Pending.into(), OffsetDateTime::now_utc().into()],
+        ),
+
+        backend @ DatabaseBackend::Sqlite => Statement::from_sql_and_values(


Testing strategy: as you noted in Slack, Postgres' locking behavior differs from SQLite's. Given how important locking is to a working queue implementation, we may want to switch to spinning up a Postgres container for relevant tests, as is done by Janus.

branlwyd · 2023-05-03T00:04:09Z

src/queue.rs

+        backend @ DatabaseBackend::Postgres => Statement::from_sql_and_values(
+            backend,
+            r#"SELECT * FROM queue
+               WHERE status = $1 AND (scheduled_at IS NULL OR scheduled_at < $2)


This queue strategy works by holding a DB lock on the relevant row for the length of processing of the queue item, so e.g. for sending email it would hold a DB lock across the API call to Postmark.

This probably works for the scale we envision. But we might want to set an appropriately-long scheduled_at value, process the record outside of a transaction (with a timeout based on scheduled_at), then update status in a second transaction. (This is effectively a "lease" over the queue item.)

Is the concern here that we will exhaust the connection pool?

Indeed, or that too-long transactions may lead to locking contention. (I think the touches to the queue table itself are unlikely to contend, as not contending is the entire purpose of the way the queries are written; but queries made while running the job itself might.)

Actually, now that I think about it, what isolation level are we running under for Postgres? I would hope for at least Repeatable Read; but if we're running at that level, a serialization failure at the very end of the transaction might lead to retrying processing of the job, which in practice would send a duplicate email. I think in that case, we need to find some way to avoid transaction retries necessitating retries of job-processing logic; one way to do so would be the suggested "lease" system, though there are other solutions too (we had previously discussed the queue states as being START, IN_PROCESS, DONE, with the IN_PROCESS state meaning "currently processing, don't pick me up, if I crash we get stuck here).

src/queue.rs

tgeoghegan · 2023-05-03T22:32:10Z

src/clients/auth0_client.rs

+fn generate_password() -> String {
+    Alphanumeric.sample_string(&mut rand::thread_rng(), 60)
+}


Our stylistic convention in Janus would be to do this as an impl Distribution<Password> for Standard, as in AuthenticationToken. You'd also then need a type like struct Password(String) so you'd have something to hang the implementation on, which might not be a bad idea anyway.

What's the advantage of this over the existing Distribution of Alphanumeric? Why reimplement code that exists in rand?

It's just a stylistic convention we landed on. TBH I imagine there are several places where this project style isn't in line with janus policies so I think we could move forward with this as it is, and consider aligning style with other Divvi Up projects in the future.

The advantage of impl Distribution<Password> for Standard, if Password is an existing type, is that this is how the rand crate "expects" to be used & it allows this type to fit in better with other uses of the API -- various other places in rand have bounds on Distribution<T>.

For example, one could type let password: Password = random() (or even let password = random() if the context is clear enough for rustc & human readers to infer the type). Or one could generate a vector of passwords via sample_iter, etc.

The implementation would be almost exactly like the body of generate_password currently, except that the Distribution<Password> version gives you an Rng to use instead of using thread_rng (in practice, I believe this RNG will be a thread_rng in most cases).

(I'm not sure a Password type is justified, but I wouldn't object to it, and if it's there we should certainly impl Distribution<Password> rather than rolling-our-own convention.)

src/entity/queue.rs

src/queue.rs

jbr · 2023-05-10T22:16:23Z

Screenshot of the admin UI

branlwyd · 2023-05-15T19:00:48Z

src/entity/task/vdaf.rs

+    pub buckets: Option<Vec<i32>>,
+}
+
+fn sorted(buckets: &Vec<i32>) -> Result<(), ValidationError> {


nit: this method makes a clone (allocating space), and sorts (taking O(n lg n) time).

what we're trying to do is determine if the list is already sorted; instead, consider just iterating through the elements and checking for that they are ordered (pairwise), which doesn't require an allocation and runs in linear time.

Are there going to be enough buckets we care about the computation in comparison to the network and db io?

We expect to have at least one use case with large histograms having tens of thousands of buckets. Longer-term, it would be good to just store the number of buckets, since that's all the aggregator cares about, but that'll probably take some changes across the VDAF spec and libprio.

Opened #102 as this is not really related to this PR aside from having moved these lines between files

branlwyd · 2023-05-15T19:02:55Z

src/entity/task/vdaf.rs

+    }
+}
+
+fn unique(buckets: &Vec<i32>) -> Result<(), ValidationError> {


nit: if you take the above suggestion, we could also fuse these two validators into something like sorted_and_unique: for each consecutive pair of elements a, b, validate that a < b. in addition to the benefits to sorted above, this would also drop the HashSet allocation from unique.

I think we wouldn't get different validation messages then

Regardless, this code isn't new in this PR, it just moved into its own vdaf.rs because task.rs was getting too big

…rity

Trillium needs a change in order to expose CloneCounter for even more graceful shutdown

Co-authored-by: David Cook <divergentdave@gmail.com>

…a result

src/queue/job/v1/send_invitation_email.rs

jbr requested a review from a team as a code owner May 2, 2023 20:42

jbr force-pushed the out-of-band-email branch from 7e35306 to bd4e5d3 Compare May 2, 2023 20:44

github-advanced-security bot found potential problems May 2, 2023

View reviewed changes

src/queue.rs Fixed Show fixed Hide fixed

src/queue.rs Fixed Show fixed Hide fixed

jbr force-pushed the out-of-band-email branch 4 times, most recently from 74fa29d to 56da705 Compare May 2, 2023 21:04

jbr mentioned this pull request May 2, 2023

Move to sea-orm-migration 0.11.3 in migration #68

Merged

jbr force-pushed the out-of-band-email branch from 56da705 to ba71c2e Compare May 2, 2023 22:51

tgeoghegan reviewed May 2, 2023

View reviewed changes

src/queue.rs Outdated Show resolved Hide resolved

src/entity/queue.rs Outdated Show resolved Hide resolved

src/queue.rs Outdated Show resolved Hide resolved

src/queue.rs Outdated Show resolved Hide resolved

src/queue.rs Outdated Show resolved Hide resolved

branlwyd reviewed May 3, 2023

View reviewed changes

jbr force-pushed the out-of-band-email branch from 88cfaa4 to 9523a84 Compare May 3, 2023 01:15

divergentdave reviewed May 3, 2023

View reviewed changes

src/queue.rs Outdated Show resolved Hide resolved

src/queue.rs Outdated Show resolved Hide resolved

jbr mentioned this pull request May 3, 2023

Kind integration feature #71

Merged

tgeoghegan reviewed May 3, 2023

View reviewed changes

tgeoghegan mentioned this pull request May 4, 2023

divviup-api in Kind #72

Merged

jbr force-pushed the out-of-band-email branch from e2b20cf to 55fce94 Compare May 10, 2023 21:25

jbr mentioned this pull request May 10, 2023

Bump @types/luxon from 3.2.0 to 3.3.0 in /app #40

Closed

jbr requested review from divergentdave and branlwyd May 11, 2023 17:42

jbr mentioned this pull request May 11, 2023

Docker improvements #86

Merged

jbr force-pushed the out-of-band-email branch from 65a6644 to bb23bb5 Compare May 12, 2023 19:41

branlwyd approved these changes May 15, 2023

View reviewed changes

jbr mentioned this pull request May 15, 2023

Make vdaf validation more computationally efficient #102

Closed

jbr added 5 commits May 15, 2023 15:32

extract out vdaf

c52bb97

add postgres-based job queue

e579eb5

just run two queue workers initially

91d00ae

use tracing::error instead of eprintln

b251984

rename queue::one to queue::dequeue_one

0e57e73

jbr and others added 12 commits May 15, 2023 15:34

naming changes aspirationally intended to improve readability and cla…

b975023

…rity

use a stopper for partial graceful shutdown

80fcc4a

Trillium needs a change in order to expose CloneCounter for even more graceful shutdown

use rand for password generation

5741a5b

Update src/queue.rs

60e932e

Co-authored-by: David Cook <divergentdave@gmail.com>

improved graceful-shutdown and sea-orm native query

c8f1f25

wrap up queue implementation into a Queue struct

18c809d

test cleanup

41fbe28

add a message id to invitation emails

31b8201

don't unwrap

6451d60

use a uuid for message id instead of hashing the job

233bc98

add api endpoints and initial ui for queue

223b8f7

change queue schema to have an error_message and child_id instead of …

7f23dbb

…a result

tgeoghegan approved these changes May 15, 2023

View reviewed changes

src/queue/job/v1/send_invitation_email.rs Outdated Show resolved Hide resolved

fixes from rebase

da44a27

jbr force-pushed the out-of-band-email branch from bb23bb5 to 47ec74d Compare May 15, 2023 23:35

use email domain from config

72159ae

jbr force-pushed the out-of-band-email branch from 47ec74d to 72159ae Compare May 15, 2023 23:44

simplify parsing logic and actually print the stringified error

1467ee2

jbr merged commit 29236f1 into main May 16, 2023
6 checks passed

jbr deleted the out-of-band-email branch May 16, 2023 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Out of band email #67

Out of band email #67

jbr commented May 2, 2023

branlwyd May 2, 2023

branlwyd May 3, 2023

jbr May 3, 2023

branlwyd May 3, 2023

tgeoghegan May 3, 2023

jbr May 3, 2023 •

edited

Loading

tgeoghegan May 4, 2023

branlwyd May 10, 2023 •

edited

Loading

jbr commented May 10, 2023

branlwyd May 15, 2023

jbr May 15, 2023

divergentdave May 15, 2023

jbr May 15, 2023

branlwyd May 15, 2023

jbr May 15, 2023

jbr May 15, 2023

Out of band email #67

Out of band email #67

Conversation

jbr commented May 2, 2023

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

jbr May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branlwyd May 10, 2023 • edited Loading

Choose a reason for hiding this comment

jbr commented May 10, 2023

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

jbr May 3, 2023 •

edited

Loading

branlwyd May 10, 2023 •

edited

Loading