Skip to content

Conversation

hyunw9
Copy link
Contributor

@hyunw9 hyunw9 commented Aug 7, 2025

What is this PR for?

If an IOException occurs while trying to remove a note from one of the repositories, the for loop would terminate immediately. This leaves the note in an inconsistent state where it is deleted from some repositories but not from others, leading to data inconsistency.

Approach

  1. The method will now attempt to remove the note from all configured repositories, even if an error occurs with one of them.
  2. Any repository that fails to remove the note is added to a failedRepos list.
  3. After iterating through all repositories, the method checks the failedRepos list. If the list is not empty, it throws a single IOException that includes information about all the repositories where the removal failed.

What type of PR is it?

Refactoring

Todos

  • - Refactor remove method
  • - Add test for modified method

What is the Jira issue?

How should this be tested?

  • Added simple integration test in NotebookRepoSyncTest.

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? - No
  • Is there breaking changes for older versions? - No
  • Does this needs documentation? - No

jongyoul
jongyoul previously approved these changes Aug 8, 2025
Copy link
Member

@jongyoul jongyoul left a comment

Choose a reason for hiding this comment

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

LGTM. I have one minor comment. What do you think we can handle if some repos fail to remove notes? There might still not be in-sync status.

@hyunw9
Copy link
Contributor Author

hyunw9 commented Aug 10, 2025

@jongyoul It would be a nicer approach. Let me show you a draft. :)

@hyunw9
Copy link
Contributor Author

hyunw9 commented Aug 11, 2025

@jongyoul I just pushed modified version. How about this approach? if it fails, we can raise exception immediately, so we can avoid out-of-sync status.

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.

2 participants