Page MenuHomePhabricator

[identity] add sync-identity-search command
ClosedPublic

Authored by will on Feb 20 2024, 7:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 5:57 PM
Unknown Object (File)
Wed, Nov 27, 5:32 PM
Unknown Object (File)
Sun, Nov 24, 1:07 AM
Unknown Object (File)
Sun, Nov 24, 12:24 AM
Unknown Object (File)
Sat, Nov 23, 11:32 PM
Unknown Object (File)
Sat, Nov 23, 9:28 PM
Unknown Object (File)
Fri, Nov 15, 11:27 AM
Unknown Object (File)
Wed, Nov 13, 12:35 AM
Subscribers

Details

Summary

This implements the sync-identity-search command in identity which syncs the opensearch search index with the latest user data from dynamodb.

Additionally verified on staging that:
Clear_index cleared the index by using an identity image that didn't clear and only restored
Scan works with 100+ items in DDB table by using 100+ test items on staging

Depends on D11124

Test Plan

Ran cargo run sync-identity-search on local and successfully confirmed search index was synced with latest dynamodb data

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 20 2024, 7:54 AM
Harbormaster failed remote builds in B27012: Diff 37374!
services/identity/src/config.rs
80 ↗(On Diff #37418)

Unsure if there's a reason why we're only letting server access ServerConfig. The sync-identity-search command currently requires the ServerConfig for the opensearch endpoint

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 21 2024, 2:31 PM
Harbormaster failed remote builds in B27049: Diff 37418!
will requested review of this revision.Feb 21 2024, 2:48 PM
varun requested changes to this revision.Feb 21 2024, 8:37 PM

can we try testing on staging and confirming that num(reserved users) + num(users table users) == num(users in index)?

services/identity/src/config.rs
80 ↗(On Diff #37418)

i think we should use the logical or || here, not bitwise or |

services/identity/src/database.rs
1000 ↗(On Diff #37418)

this should be removed

services/identity/src/main.rs
24 ↗(On Diff #37418)

can we rename this module? sync is a std module

99 ↗(On Diff #37418)

can you remind me what consume_error does?

services/identity/src/sync.rs
34 ↗(On Diff #37418)

how can we confirm that this works?

91 ↗(On Diff #37418)

can you explain why we need this header? also why is this different than the header on line 56?

This revision now requires changes to proceed.Feb 21 2024, 8:37 PM
services/identity/src/main.rs
99 ↗(On Diff #37418)

It matches and error! logs if error result and discards ok result.

Here's the code:

pub fn consume_error<T>(result: Result<T, Error>) {
  match result {
    Ok(_) => (),
    Err(e) => {
      error!("{}", e);
    }
  }
}
services/identity/src/sync.rs
34 ↗(On Diff #37418)

If you mean confirming inside the code, we can query the opensearch domain and check if the number of hits returned is zero. I can add in the next rebase

91 ↗(On Diff #37418)

application/x-ndjson is for the _bulk endpoint. The _delete_by_query endpoint only requires application/json.

From what I understand, elastic and opensearch uses x-ndjson for performance purposes. In most guides, x-ndjson is used including on the official opensearch api docs.

The latest version of Elastic seems to support json as well but x-ndjson seems to be the most generally supported.

services/identity/src/config.rs
80 ↗(On Diff #37418)

I believe it's actually | for match pattern. || gives me a compile error and requires |

please make sure to test that:

  • clear_index actually clears the index
  • scan works with 100+ items in DDB table
services/identity/src/sync.rs
91 ↗(On Diff #37418)

we should use reqwest::header::CONTENT_TYPE above as well, though, right?

This revision is now accepted and ready to land.Feb 22 2024, 8:26 PM

please make sure to test that:

  • clear_index actually clears the index
  • scan works with 100+ items in DDB table

Verified this during testing. Should be all set

services/identity/src/sync.rs
91 ↗(On Diff #37418)

how about this comment?

services/identity/src/sync.rs
91 ↗(On Diff #37418)

Yep. Thanks

will marked 2 inline comments as done.Feb 25 2024, 7:06 PM
This revision was landed with ongoing or failed builds.Feb 26 2024, 12:58 PM
This revision was automatically updated to reflect the committed changes.