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

Chore/fix user query #2296

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Chore/fix user query #2296

merged 3 commits into from
Mar 18, 2024

Conversation

gweiying
Copy link
Contributor

@gweiying gweiying commented Mar 18, 2024

Problem

Optimising our current query to check if short link exists. Previous query was doing an unnecessary join on the users table, causing the query duration to spike.

Screenshot 2024-03-18 at 5 34 26 PM

Solution

Replaced query to only check in urls table instead of joining urls with user table.

Before & After Screenshots

BEFORE:
Screenshot 2024-03-18 at 5 44 04 PM

AFTER:
Screenshot 2024-03-18 at 5 35 38 PM

Tests

  • Ran load tests on staging to check if query duration reduced.

E2E Manual tests

  • If short link is already used, should not be able to create short link
  • When creating new links via APIs, should be able to create shortened link without specifying short link
  • When creating new links via APIs, if short link is already used, should not be able to create short link

@gweiying gweiying marked this pull request as ready for review March 18, 2024 09:31
@gweiying gweiying requested a review from halfwhole March 18, 2024 09:44
Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

lgtm!

// Cache lookup
// if long url does not exist, throws error
const longUrl = await this.getLongUrlFromCache(shortUrl)
return !longUrl
Copy link
Contributor

@halfwhole halfwhole Mar 18, 2024

Choose a reason for hiding this comment

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

nit: small optimisation but unnecessary

Suggested change
return !longUrl
return true

edit should be false OOPS

@gweiying gweiying merged commit 620a2b0 into develop Mar 18, 2024
24 checks passed
@gweiying gweiying deleted the chore/fix-user-query branch March 18, 2024 10:03
@gweiying gweiying restored the chore/fix-user-query branch March 18, 2024 10:15
@gweiying gweiying mentioned this pull request Mar 18, 2024
Comment on lines +193 to +195
const url = await Url.findOne({
where: { shortUrl },
})

Choose a reason for hiding this comment

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

/nit/micro-optimization: since we only care for an exists, you could reduce the selection to just the shortUrl field. Otherwise the query seems to be sent like this atm (according to the traces):

SELECT shortUrl, longUrl, state, isFile, contactEmail, description, source, tagStrings, createdAt, updatedAt, userId FROM urls WHERE url . shortUrl = ?
Suggested change
const url = await Url.findOne({
where: { shortUrl },
})
const url = await Url.findOne({
attributes: ['shortUrl'],
where: { shortUrl },
})

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.

None yet

3 participants