Bug 411422 - Improve updating and patch set loading behavior
Summary: Improve updating and patch set loading behavior
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 2.0.1   Edit
Assignee: Miles Parker CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-21 18:38 EDT by Miles Parker CLA
Modified: 2013-07-12 12:22 EDT (History)
3 users (show)

See Also:


Attachments
screenshot with the error (101.90 KB, image/png)
2013-07-09 15:49 EDT, Tomasz Zarna CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Miles Parker CLA 2013-06-21 18:38:23 EDT
Currently, when you open a review that has been opened before, the patch set isn't loaded by default. We did that as part of the effort to simplify the model update process and move to a pure TaskData based approach.

But this leads to annoying behavior, in that you have to manually load the task. This is due to the fact that we no longer ask for an update when we first load a model. Although this is reasonable to fit in with update strategy, the problem is that we don't have the remote object which we need for compare operations and the like.

So I'm going to look at grabbing the remote object and updating anyway, even though to some extent that duplicates the role of Task Data.
Comment 1 Miles Parker CLA 2013-06-21 19:25:01 EDT
https://git.eclipse.org/r/13998

I also noticed a tricky issue with TaskData where we often are trying to update at the same time the user has just trigerred a retrieval. I think fixing this should improve retrieval behavior.
Comment 2 Steffen Pingel CLA 2013-06-25 13:34:17 EDT
I occasionally get warnings like this in the log and the editor fails to load a particular patch set. While I'm not sure what is causing the problem (could be comment 1) the error message also doesn't tell me anything beyond what I already now. It should include the exception that caused the operation to fail (i.e. the IOException or whatever).  

Couldn't update model.

org.eclipse.core.runtime.CoreException: Couldn't obtain patch set detail for 2. Check remote connection.
	at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentIdRemoteFactory.pull(PatchSetContentIdRemoteFactory.java:53)
	at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentIdRemoteFactory.pull(PatchSetContentIdRemoteFactory.java:1)
	at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer.pull(RemoteEmfConsumer.java:164)
	at org.eclipse.mylyn.reviews.core.spi.remote.JobRemoteService$1.run(JobRemoteService.java:60)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Comment 3 Miles Parker CLA 2013-06-25 13:44:52 EDT
(In reply to comment #2)
> I occasionally get warnings like this in the log and the editor fails to load a
> particular patch set. While I'm not sure what is causing the problem (could be
> comment 1) the error message also doesn't tell me anything beyond what I already
> now. It should include the exception that caused the operation to fail (i.e. the
> IOException or whatever).

Yeah, the error is really misleading. The problem is that we don't really know at this point why we couldn't retrieve the patch set detail.  This is a bit of an odd case, because we actually retrieve all of that information when we retrieve the Review (Gerrit Change). However, each PatchSet has its own RemoteConsumer, so that we can map the model object (IReviewItemSet) to the remote object (PatchSetDetail). So all the "pull" is doing at this point is returning the PatchSetDetail that "should" already be there.

We're just surmising that it was because of an early connection error. But if we're getting this error and we don't see a previous connection issue, then there is something else going wrong here and I'm not sure what it is. That's what I need to investigate.
Comment 4 Miles Parker CLA 2013-07-02 16:14:24 EDT
Please review https://git.eclipse.org/r/#/c/13998/11 . I'd like to get this change in as it fixes some real annoyances. As long as it doesn't break anything else we can always add additional improvements on an on-going basis.
Comment 5 Miles Parker CLA 2013-07-09 15:04:46 EDT
Tomek, you still have a -1 on this review. Please see my comment and verify that issue.
Comment 6 Tomasz Zarna CLA 2013-07-09 15:49:33 EDT
Created attachment 233295 [details]
screenshot with the error

On the review I said: 
"Not sure if this is supposed to be fixed by the patch but I still get an error saying "Cannot Fetch. Try re-synchronizing the review task. If that fails, there may be a problem with your repository connection." if I try to fetch a patch set while the review is being retrieved."

And I'm still able to reproduce it. It doesn't happen every time, I guess I had to try 3 reviews to see it. Here are the steps:

1. Open a review in editor
2. Make sure a job retrieving review is running
3. Press Fetch

If I close the error dialog and press Fetch again it works as expected.
Comment 7 Miles Parker CLA 2013-07-09 16:51:28 EDT
(In reply to comment #6)
> If I close the error dialog and press Fetch again it works as expected.

Thanks. I'll take a look at this but prob. not until tmrw.
Comment 8 Miles Parker CLA 2013-07-10 19:01:35 EDT
(In reply to comment #6)
> Created attachment 233295 [details]
> And I'm still able to reproduce it. It doesn't happen every time, I guess I had
> to try 3 reviews to see it. Here are the steps:

Hmm..I tried it a bunch of times and still no luck. Perhaps because I'm closer to my test server.. but even if I click when the pathct set contents aren't listed, it doesn't happen. At this point, I'm inclined to get this change in and open another bug for your issue. Does that make sense to you?
Comment 9 Tomasz Zarna CLA 2013-07-11 06:58:51 EDT
(In reply to comment #8)
> Does that make sense to you?

Fine with me, as long as it's going to be targeted for 2.0.1. In the meantime, maybe we should add some tracing to track the culprit more easily?

I also noticed that for some reviews the "Retrieving Review XXX" and "Retrieving Review XXX, Patch Set YYY" jobs are ran, even when the patch set is already there.

I found at least one review where "Synchronize to retrieve task data" msg stuck forever and the "Retrieving Review XXX" job wasn't fired.

Comparing two patches sometimes takes forever = doesn't work. It either opens instantly or never. Luckily, the job can be cancelled.
Comment 10 Miles Parker CLA 2013-07-11 13:21:09 EDT
(In reply to comment #9)
> Fine with me, as long as it's going to be targeted for 2.0.1. In the meantime,
> maybe we should add some tracing to track the culprit more easily?

Interesting idea. Do you have suggestions about how to implement that?

> I also noticed that for some reviews the "Retrieving Review XXX" and "Retrieving
> Review XXX, Patch Set YYY" jobs are ran, even when the patch set is already
> there.

Well, they will be, when you have refreshed the task.

> I found at least one review where "Synchronize to retrieve task data" msg stuck
> forever and the "Retrieving Review XXX" job wasn't fired.

If you find that case, please record it.

> Comparing two patches sometimes takes forever = doesn't work. It either opens
> instantly or never. Luckily, the job can be cancelled.

Yes, I'm not sure if that's something that is happening on the backend or now. It would be good to isolate that.
Comment 11 Miles Parker CLA 2013-07-11 13:24:43 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > Does that make sense to you?
> 
> Fine with me, as long as it's going to be targeted for 2.0.1. 

bug 412782: Intermittent fetching issues.
Comment 12 Tomasz Zarna CLA 2013-07-12 07:11:02 EDT
(In reply to comment #10)
> > I found at least one review where "Synchronize to retrieve task data" msg stuck
> > forever and the "Retrieving Review XXX" job wasn't fired.
> 
> If you find that case, please record it.

What do you mean by 'record it'? What kind of information would help you investigate this?
Comment 13 Miles Parker CLA 2013-07-12 12:22:29 EDT
(In reply to comment #12)
> (In reply to comment #10)
> > > I found at least one review where "Synchronize to retrieve task data" msg stuck
> > > forever and the "Retrieving Review XXX" job wasn't fired.
> > 
> > If you find that case, please record it.
> 
> What do you mean by 'record it'? What kind of information would help you
> investigate this?

You mentioned "one review". I was thinking that you were referring to a specific review. If so, please send me the details. Otherwise, let's see if we can find a pattern and open a bug for it. We should treat any remaining issues separately.