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

[MRESOLVER-305] Handle blocked state at connector level #228

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Dec 9, 2022

As blocked is really about block remote access. Locally cached things should not be affected, but current solution did not cover all execution paths.


https://issues.apache.org/jira/browse/MRESOLVER-305

As blocked is really about block remote access. Locally cached
things should not be affected, but current solution did not cover
all execution paths.

---

https://issues.apache.org/jira/browse/MRESOLVER-305
@cstamas cstamas requested review from kwin and hboutemy December 9, 2022 15:09
@cstamas cstamas self-assigned this Dec 9, 2022
@cstamas cstamas marked this pull request as ready for review December 9, 2022 15:33
{
if ( repository.getMirroredRepositories().isEmpty() )
{
throw new NoRepositoryConnectorException( repository, "Blocked repository: " + repository );
Copy link
Member

Choose a reason for hiding this comment

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

we should probably have some tests, because to me it is not clear where this exception is caught and logged. Does it prevent other remote repos from being tried or not?

Copy link
Member Author

@cstamas cstamas Dec 9, 2022

Choose a reason for hiding this comment

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

if you look from where this block was removed, it is in same try-catch where this method is invoked (and throws this exception). This move basically prevents any "remote access" to blocked repositories, not just for one case like originally. But yes, I agree, we do need test, I thought to reuse existing ones (are there any?)

Copy link
Member

@kwin kwin Dec 9, 2022

Choose a reason for hiding this comment

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

But if I have one blocked remote repo and one not-blocked one I expect the latter to be used instead (if possible). Resolving should only fail if the requested artifact/metadata could not be delivered from any non-blocked remote repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as connector provider provides connector for one given remote repo (is param on method), so it will happily provide connector instance for other non-blocked remote repositories....

Copy link
Member

@kwin kwin Dec 9, 2022

Choose a reason for hiding this comment

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

Ok, sorry, now I see the related catch block in

and
and indeed that should work just as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in short: resolver will refuse to provide any connector for blocked RemoteRepository instances. Everywhere.

Copy link
Member

@kwin kwin left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@cstamas cstamas added this to the 1.9.3 milestone Dec 11, 2022
@cstamas cstamas merged commit c5619a6 into apache:master Dec 22, 2022
@cstamas cstamas deleted the MRESOLVER-305-blocked-repository branch December 22, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants