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 BSA validation job #2356

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Add BSA validation job #2356

merged 3 commits into from
Mar 8, 2024

Conversation

weiminyu
Copy link
Collaborator

@weiminyu weiminyu commented Mar 6, 2024

Add the BsaValidateAction class with a first check (for inconsistency between downloaded and persisted labels).


This change is Reviewable

return null;
}

void emailValidationResults(String job, ImmutableList<String> errors) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'job' is never used.
return null;
}

void emailValidationResults(String job, ImmutableList<String> errors) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'errors' is never used.
Copy link
Collaborator Author

@weiminyu weiminyu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @Github-advanced-security[bot])


core/src/main/java/google/registry/bsa/BsaValidateAction.java line 93 at r1 (raw file):

Previously, github-advanced-security[bot] wrote…

Useless parameter

The parameter 'job' is never used.

Show more details

Params will be used.


core/src/main/java/google/registry/bsa/BsaValidateAction.java line 93 at r1 (raw file):

Previously, github-advanced-security[bot] wrote…

Useless parameter

The parameter 'errors' is never used.

Show more details

Params will be used.

@jianglai
Copy link
Collaborator

jianglai commented Mar 6, 2024

Copy link
Collaborator Author

@weiminyu weiminyu left a comment

Choose a reason for hiding this comment

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

Done.

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @Github-advanced-security[bot])

@jianglai
Copy link
Collaborator

jianglai commented Mar 7, 2024

We should add a test to make sure that there's no divergence going forward. Not in this PR, though.

Copy link
Collaborator

@sarahcaseybot sarahcaseybot left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 5 of 10 files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot], @jianglai, and @weiminyu)


core/src/main/java/google/registry/bsa/BsaValidateAction.java line 99 at r2 (raw file):

  ImmutableList<String> checkBsaLabels(String jobName) {
    ImmutableSet<String> downloadedLabels = fetchDownloadedLabels(jobName);
    ImmutableSet<String> persistedLabels = fetchPersistedLabels(/* batchSize= */ 500);

You should make the batch size configurable


core/src/main/java/google/registry/bsa/BsaValidateAction.java line 118 at r2 (raw file):

          String.format(
              "Found %d unexpected labels in the DB. Examples: [%s]",
              missingLabels.size(), examples);

This should use unexpectedLabels


core/src/main/java/google/registry/bsa/persistence/Queries.java line 55 at r2 (raw file):

                        "SELECT b.label FROM BsaLabel b WHERE b.label > :lastRead ORDER BY b.label",
                        String.class)
                    .setParameter("lastRead", lastRead.orElse(""))

Does this work when lastRead is empty? Does "... WHERE b.label > ORDER BY b.label" work?

Copy link
Collaborator Author

@weiminyu weiminyu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 10 files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot], @jianglai, and @sarahcaseybot)


core/src/main/java/google/registry/bsa/BsaValidateAction.java line 99 at r2 (raw file):

Previously, sarahcaseybot wrote…

You should make the batch size configurable

Done.


core/src/main/java/google/registry/bsa/BsaValidateAction.java line 118 at r2 (raw file):

Previously, sarahcaseybot wrote…

This should use unexpectedLabels

Done.


core/src/main/java/google/registry/bsa/persistence/Queries.java line 55 at r2 (raw file):

Previously, sarahcaseybot wrote…

Does this work when lastRead is empty? Does "... WHERE b.label > ORDER BY b.label" work?

When lastRead is empty the query will be WHERE b.label > '' ORDER BY b.label.
Empty string goes before any other string.

One of the new method in QueriesTest.java tests this.

Copy link
Collaborator

@sarahcaseybot sarahcaseybot left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 9 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @Github-advanced-security[bot] and @jianglai)

Add the BsaValidateAction class with a first check (for inconsistency
between downloaded and persisted labels).
@weiminyu weiminyu added this pull request to the merge queue Mar 8, 2024
Merged via the queue into google:master with commit 34a8a94 Mar 8, 2024
7 of 8 checks passed
@weiminyu weiminyu deleted the add-bsa-validate branch March 8, 2024 23:11
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.

3 participants