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

Provide a way to force an update of cahed p2 repositories when using version ranges in .target files #1514

Closed
phermsdorf opened this issue Dec 13, 2024 · 13 comments

Comments

@phermsdorf
Copy link

Hi,

as discusessed in #1513 , it would be great to be able force an update of p2 repositories (aka. invalidate the cache) to consume eg. updated SNAPSHOT builds.

Currently the only working way seems to restart the complete IDE.

Thanks you.

Bye Peter

@merks
Copy link
Contributor

merks commented Dec 13, 2024

@HannesWell @laeubi

I think a method like this in P2Utils called from the appropriate place(s) would do the trick:

	public static void flushRepositories() throws CoreException {
		if (getArtifactRepositoryManager() instanceof AbstractRepositoryManager<?> repository) {
			repository.flushCache();
		}
		if (getRepoManager() instanceof AbstractRepositoryManager<?> repository) {
			repository.flushCache();
		}
	}

Of course the next question is, where are the appropriate places?

@laeubi
Copy link
Contributor

laeubi commented Dec 13, 2024

We already have

grafik

beside that it is unclear for me why this is needed, P2 has no notation of SNAPSHOT, from the mentioned discussion it more looks like the Refresh button in the Target editor should force update the the repositories like its done with Refresh in the Target Platform preferences.

@merks
Copy link
Contributor

merks commented Dec 13, 2024

I don’t see relationship between what you show and the calls that flush the in-memory, already-loaded repositories so that they are reloaded from the remote URI. If the already-loaded in-memory repository is reused, it will simply download the older artifacts again. This was annoying behavior with oomph’s tasks, i.e., not actually reading the latest repo content but rather using the state it was in earlier. Only a restart solves that problem, as described in the discussion.

@laeubi
Copy link
Contributor

laeubi commented Dec 13, 2024

I don’t see relationship between what you show and the calls that flush the in-memory, already-loaded repositories so that they are reloaded from the remote URI.

Your snippet will flush all caches, so if we want something like that I would expect it to be near by the bundle pool preferences with a clean button.

This was annoying behavior with oomph’s tasks, i.e., not actually reading the latest repo content but rather using the state it was in earlier. Only a restart solves that problem, as described in the discussion.

In the context of a target platform it should be enough to flush the cache of the repositories in question when hitting the "Reload" button. This is probably not done as of today, but I don't think we need a new UI for that.

@merks
Copy link
Contributor

merks commented Dec 13, 2024

I would personally hope that when I ask to refresh the target platform that the behavior will not depend on what may or may not already be in memory. I would not expect to hunt for a place in the preferences for a button. So I definitely don’t suggest a new button. If we can’t selectively flush the cache, hit it with a big hammer. Given composite repositories and repositories with references, knowing what to flush selectively is a hard problem, so I propose the big hammer.

@HannesWell
Copy link
Member

If we can’t selectively flush the cache, hit it with a big hammer. Given composite repositories and repositories with references, knowing what to flush selectively is a hard problem, so I propose the big hammer.

From reading the tooltip of the Refresh button I actually would have expected that exactly that would happen: Discard all metadata and get the latest ones available (from a mutable p2-repo).
grafik

If one only wants to re-resolve the target-definition one should simply not hit that button but trigger the resolve differently e.g. by changing tabs and going back to the Definition tab (I know that's a bad UX workflow and should be improved too, but that's a way that works a.t.m.).
So yes I'm also in favor of the big hammer for the Refresh-button.

In general I already wondered for some time if P2 itself could become smarter when it comes to detecting if the content of a mutable repository changed. I.e. my idea is to store a unique number in the p2.index that changes whenever the repo has changed. It could be e.g. a hash of the content/artifact.xml or just a timestamp of the creation.
This way only the p2.index file would have to be fetched and one could determine if that number and thus the repo has changed or not. And act accordingly.
For a composite one would still have to check all children individually since the number is not propagated to composite, but at least one would not have to re-fetch the metadata for the composite if just one child changed.

@laeubi
Copy link
Contributor

laeubi commented Dec 14, 2024

In general I already wondered for some time if P2 itself could become smarter when it comes to detecting if the content of a mutable repository changed. I.e. my idea is to store a unique number in the p2.index that changes whenever the repo has changed.

The problem with such thing is that it need to be rolled out and don't help with all setups as there are multiple ways to create a site. In any case, actually we have already such thing (at least for HTTP, FTP would be possible as well) but there is one big provider (if not the biggest one) that explicitly renders this feature useless:

https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/1039

because of this Tycho even goes further and caches content for 60minutes (but has the -U option to force recheck).

Anyways obviously P2 can know that if it works after a restart of eclipse, so I think it is more a matter to bypass the (in memory) cache, that is mainly used to not load a repository more than once in a session.

@merks
Copy link
Contributor

merks commented Dec 14, 2024

As Christoph suggests, there is a local file cache and an in-memory cache. They are two different things. The local file cache's time stamp is compared to the remote timestamp such that it's downloaded again as needed. This generally works well. Sometimes the servers are a little slow (some delay) about actually serving up new content, but that cannot be helped on the client side. It is the reusing of the in-memory repositories, which are of course there to avoid loading the same thing repeatedly, that cause problems, i.e., unexpected, hard-to-reproduce behavior because of course neither the local file cache nor the remote source is consulted.. So I think that operations that explicitly ask for the target platform to be reloaded or re-resolved should just flush the repository manager caches; Oomph targlet resolution and p2 task resolution (and even the repository explorer) do exactly that because one simply expects and wants such behavior.

@laeubi
Copy link
Contributor

laeubi commented Dec 14, 2024

I have now created a PR

So I think that operations that explicitly ask for the target platform to be reloaded or re-resolved should just flush the repository manager caches; Oomph targlet resolution and p2 task resolution (and even the repository explorer) do exactly that because one simply expects and wants such behavior.

The problem with what people "wants" is that this often changes, e.g. people also "wants" that target editor load fast... so flushing caches "just in case" will decrease performance for these use-cases.

@HannesWell
Copy link
Member

So I think that operations that explicitly ask for the target platform to be reloaded or re-resolved should just flush the repository manager caches; Oomph targlet resolution and p2 task resolution (and even the repository explorer) do exactly that because one simply expects and wants such behavior.

The problem with what people "wants" is that this often changes, e.g. people also "wants" that target editor load fast... so flushing caches "just in case" will decrease performance for these use-cases.

The target-editor should load and resolve fast yes and I think that should not be changed.
But the Refresh button should do a 'deep' reload of all metadata, yes, as the tool-tip indicates.
If that button doesn't reload deeply it's IMO point-less and if it needs time, then so be it. If one doesn't want to spend the time, one should simply not press it.
Because what's the purpose of the Refresh button if it just does a 'light' refresh? TBH, I don't know exactly what that button is doing now respectively haven't looked it up yet.

@laeubi
Copy link
Contributor

laeubi commented Dec 14, 2024

Because what's the purpose of the Refresh button if it just does a 'light' refresh? TBH, I don't know exactly what that button is doing now respectively haven't looked it up yet.

The button informs the target location to throw away its state and then resolves the location again, what specifically is performed is dependent on the location implementation. So as mentioned before (and done in the PR) the Location implementation should do something sensible and not "the button". For example consider a DirectoryLocation can change its contents on file system and then you need to press Refresh to take this into account. M2e re-resolves the maven artifacts (that might have changed due to install or configuration change) and so on ...

@laeubi
Copy link
Contributor

laeubi commented Dec 14, 2024

I have checked the refresh of IU locations and the actually already perform the operation to refresh the repositories, so I don't see much PDE can do here.

With

in place I therefore closing this, if there are still issues please provide steps how to reproduce the problem.

@merks
Copy link
Contributor

merks commented Dec 14, 2024

Yes, I think we can assume this is working well now.

@merks merks closed this as completed Dec 14, 2024
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

No branches or pull requests

4 participants