Community
Participate
Working Groups
This is a bug to round up all of the life-cycle issues for the editor the current release -- the # of bugs aren't really manageable individually and these issues should all have the same root causes. We'll reetain the orginal bugs so that we can be sure we've addressed all issues. Any bugs later determined not to be caused by life-cycle issues will be removed from dependencies.
https://git.eclipse.org/r/#/c/13154/
Miles, can you provide some detail what you suspect the cause of the problem is and how it can be fixed?
Sure. So the basic issue is that we need to construct and update a complex set of nested controls (editor, sections, etc..) and do so in a way that matches expectations of task editors workflow (what happens when users click refresh, etc..), still works with Mylyn Tasks retrieval (so that comments and dates work proeperly) but also meshes with the more direct communication provided through remote Gerrit API calls as mediated by the new Remote API. Within the Remote API context, we need to retrieve model objects a) when they don't exist at all on local machine or b) when the user requests them, or c) for creating buttons (since this *always* requires a live connection to backend). To give an idea of the complexity of this note that we might have any one of these situations -- note that this is just for patch sets, which is the most complex case: a) Have a local Review model, but an non-existent (say new) patchset. In this case, we need to do a live retrieval when user first opens the patch set, which may or may not be when the editor is opened. Note that we *will* have some of the patch set data from the review retireval, and so we need to display that part, even when we don't have a connection to Gerrit. b) Have an existing patchset but no connection to Gerrit. (In this case, the buttons cannot be displayed.) c) Have an existing patchset, but a connection to Gerrit. (In this case, the patch set is displayed, but the buttons have to be created as part of a seperate remote update!) d) Have changes to that patchset, such as comments, that need to be updated after user clicks refresh button. e) Have the patch set notified of changes when another process (such as review navigator) causes the patch set to be refreshed. f) There are probably one or two cases that I missed here... So the problem is that this is not all working well for all cases. This is all compounded by not having a good testing approach for the overall editor life-cycle. We have the classic issue where fixing one issue can introduce regressions elsewhere, and it's difficult to cover all cases in manual testing. Before, all of this code was hand written inside of the editor itself, which was nearly impossible to understand from a maintenance POV especially once I replaced the gerrit stuff w/ Remote API calls. I tried to regularize all of this by creating the Remote Clients, but I'm not sure that was a good idea, because there end up being so many special cases anyway. So, there a few ways to approach this. 1) I think no matter what we should iterate a bit more on the current approach, as frustrating as it is, perhaps we're close and it's worth a day or two to find out. 2) We have very limited time, but we may need to step back a bit and really capture the entire life-cycle more formally, perhaps w/ an Activity diagram or something. 3) Think about how we might dramatically simplify the life-cycle, by say making all Gerrit reviews "live", pre-empting the existing lazy updating scheme. hth explain where we are..
With the latest patch set I still ran into problems refreshing the editor. Pressing refresh several times eventually resulted in exceptions being logged: Caused by: java.lang.NullPointerException at org.eclipse.mylyn.internal.gerrit.ui.factories.SubmitUiFactory.isExecutable(SubmitUiFactory.java:38) at org.eclipse.mylyn.reviews.ui.spi.factories.AbstractUiFactory.createControl(AbstractUiFactory.java:60) at org.eclipse.mylyn.reviews.ui.spi.factories.AbstractUiFactoryProvider.createButtons(AbstractUiFactoryProvider.java:38) at org.eclipse.mylyn.reviews.ui.spi.editor.ReviewSetContentSection.updateButtons(ReviewSetContentSection.java:348) at org.eclipse.mylyn.reviews.ui.spi.editor.ReviewSetContentSection$2.update(ReviewSetContentSection.java:136) at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfClient.checkUpdate(RemoteEmfClient.java:52) at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfClient.updated(RemoteEmfClient.java:84) at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer$ConsumerAdapter.notifyChanged(RemoteEmfConsumer.java:85) at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:374) at org.eclipse.emf.common.notify.impl.NotificationChainImpl.dispatch(NotificationChainImpl.java:98) at org.eclipse.emf.common.notify.impl.NotificationChainImpl.dispatch(NotificationChainImpl.java:86) at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer.applyModel(RemoteEmfConsumer.java:237) at org.eclipse.mylyn.reviews.core.spi.remote.JobRemoteService$2$1.run(JobRemoteService.java:77) at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:164) I'm also seeing patch sets getting duplicated (bug 409306).
(In reply to comment #3) > Sure. So the basic issue is that we need to construct and update a complex set > of nested controls (editor, sections, etc..) and do so in a way that matches > expectations of task editors workflow (what happens when users click refresh, > etc..), still works with Mylyn Tasks retrieval (so that comments and dates work > proeperly) but also meshes with the more direct communication provided through > remote Gerrit API calls as mediated by the new Remote API. Is the review model refresh always driven by the task editor or does it hook into the task refresh as well? > b) Have an existing patchset but no connection to Gerrit. (In this case, the > buttons cannot be displayed.) Why handle buttons as a special case? Why not always display them and fail on submission. I would expect the model to have all information required to render buttons since eventually the editor should support full offline modifications including workflows. > c) Have an existing patchset, but a connection to Gerrit. (In this case, the > patch set is displayed, but the buttons have to be created as part of a seperate > remote update!) I'm not quite getting why this needs to be separate. Why can't that be retrieved when getting the patch set or patch set content? > 3) Think > about how we might dramatically simplify the life-cycle, by say making all > Gerrit reviews "live", pre-empting the existing lazy updating scheme. That sounds right to me. I would be careful about caching patch set content but it seems reasonable to me to cache all other information as part of task retrieval as it was done in 1.0. Even if we sacrifice some performance advantage we will be much better of with a radically simplified refresh strategy.
(In reply to comment #5) > Is the review model refresh always driven by the task editor or does it hook > into the task refresh as well? No, it is only manually refreshed through the task editor (the user can initiate this from Review explorer as well). My understanding was that this was the expected approach, i.e. the task framework should only refresh large sets of data when the user explicitly requests. > > b) Have an existing patchset but no connection to Gerrit. (In this case, the > > buttons cannot be displayed.) > > Why handle buttons as a special case? Why not always display them and fail on > submission. I would expect the model to have all information required to render > buttons since eventually the editor should support full offline modifications > including workflows. Well, the issue is that we don't know which buttons to provide. For example, you can submit some patch sets only if you have credentials.. And if you don't have a remote connection at all, it doesn't seem to make sense to do that. Also, as the errors attest, you actually need the remote data, as the exceptions attest. Though we could trap for those and/or make the buttons non-executable. I thought it better just not to show them at all, but hmm... > > c) Have an existing patchset, but a connection to Gerrit. (In this case, the > > patch set is displayed, but the buttons have to be created as part of a > seperate > > remote update!) > > I'm not quite getting why this needs to be separate. Why can't that be retrieved > when getting the patch set or patch set content? We do. But note that we may have the patch set items (because they're coming from the local model) but not the remote. BTW, an additional complication that I didn't state is that we actually have multiple states that we can be in.. 1. We can have the Review task data, but no model. 2. We can have the Review model, but no Remote object (the Gerrit API objects) *yet*. 3. We can have the Review model, but can't obtain the Remote object at all *yet*. 4. We can have the Remote object, but not the Review model (!) simply because the model hasn't been populated yet. (This case should be handled by the framework, and shouldn't be a concern for consumers.) > > 3) Think > > about how we might dramatically simplify the life-cycle, by say making all > > Gerrit reviews "live", pre-empting the existing lazy updating scheme. > > That sounds right to me. I would be careful about caching patch set content but > it seems reasonable to me to cache all other information as part of task > retrieval as it was done in 1.0. Even if we sacrifice some performance advantage > we will be much better of with a radically simplified refresh strategy. Can you say a bit more about what the strategy would be here? Originally I had it setup to just not get anything but the bare task id and so on that we don't show any of the cached model data when we don't have the remote. I'd love to do it this way, though this could be a pretty big change at this stage and mean that we grabbed data for every single review in the queries, right? At that point though, we could do away with the integration into the editor refresh cycle (which is really a bit hacky feeling if you look at refresh in the review task editor code you'll see what I mean)?
Miles, stepping way, way back, because I know very little about how the connector works, could you explain, at a high level, the reasons for having such a complex arrangement? Why not have all needed information (other than patch set contents) in the task data, like any other connector, and fetch the patch set contents on demand? What is special about Gerrit that requires this complexity in the connector, or what are we trying to achieve with it?
(In reply to comment #7) > Miles, stepping way, way back, because I know very little about how the > connector works, could you explain, at a high level, the reasons for having such > a complex arrangement? Why not have all needed information (other than patch set > contents) in the task data, like any other connector, and fetch the patch set > contents on demand? What is special about Gerrit that requires this complexity > in the connector, or what are we trying to achieve with it? It's a good question... The issue is that we want/need to persist the patch sets as well. But even if we didn't, we'd need a notification mechanism for that, we'd have to put them in a model, and the communication would have to be asyncronous. It also has to work across views, so we need that notification there. The Gerrit API itself is also a strange mix of async and direct communication. And I think persistence is really valuable as it saves a tremendous amount of time for users who are reopening exsiting tasks -- but whether we persist it or not isn't the main issue anyway. Also, I think a decision was made that we didn't want to put the model handling within the Task Data side, but I can't remember offhand what the reasoning was there. It may have been that we don't have access to everything we need at that point. I can take another look at that -- though I'd hate to tear a bunch of stuff out at this late date it would .
We would also like to have an offline cache for task attachments. I don't think persisting the patch sets should require this complexity. Is it just an unavoidable result of a complicated Gerrit API, or is it a result of some design decisions we've made, e.g. using EMF?
I was really unclear above. Let me try again. The first issue is that we'd like to persist the patch sets (but we don't have to) -- the real issue is that we need to load the patch sets lazily, on user demand. And it's not like double-clicking on an attachment, it's much more tightly integrated into the user interactions -- sometimes you need to open them automatically (like the last one) sometimes you don't, sometimes it can fail (if you don't have a connection or if the patch set is missing, see bug 399434) and these all have to be coordinated across multiple reviews. It is *not* a result of using EMF -- I can state that categorically -- any backing store would have these same complexities. Note that we're dealing essentially with both server side pull and server side push. So yes, the compleity of the Gerrit API, the complexity of the Mylyn Task Editor API and task life-cycle, added to this the new Rmeote API, they're all implicated.
I see. Why does it have to be coordinated across multiple reviews?
(In reply to comment #6) > (In reply to comment #5) > > Is the review model refresh always driven by the task editor or does it hook > > into the task refresh as well? > > No, it is only manually refreshed through the task editor (the user can initiate > this from Review explorer as well). My understanding was that this was the > expected approach, i.e. the task framework should only refresh large sets of > data when the user explicitly requests. Does that mean users have to press the refresh button in order to get the changes for a review regardless of task synchronizations (happening in the background)?
(In reply to comment #6) > Well, the issue is that we don't know which buttons to provide. For example, you > can submit some patch sets only if you have credentials.. And if you don't have > a remote connection at all, it doesn't seem to make sense to do that. You would need a notion of an "offline" status in order to implement disabling/hiding of controls based on server availability but Mylyn does not have that by design. It seems strange to me to hide buttons based on connectivity at the time of opening the editor. User may reconnect later and wish to submit without having to reopen the editor. Generally, part of the value of caching data locally is that I don't have to waiting for a repository connection before triggering an interaction but that seems broken in the current review editor.
(In reply to comment #12) > Does that mean users have to press the refresh button in order to get the > changes for a review regardless of task synchronizations (happening in the > background)? Yes, which feels kind of weird and sub-optimal to me given that we can easily get that data, but seems consistent with the way other connectors handle "non-partial" data. (In reply to comment #13) > (In reply to comment #6) > > Well, the issue is that we don't know which buttons to provide. For example, you > > can submit some patch sets only if you have credentials.. And if you don't have > > a remote connection at all, it doesn't seem to make sense to do that. > > You would need a notion of an "offline" status in order to implement > disabling/hiding of controls based on server availability but Mylyn does not > have that by design. It seems strange to me to hide buttons based on > connectivity at the time of opening the editor. User may reconnect later and > wish to submit without having to reopen the editor. Generally, part of the value Yes, but then how do you distinguish between the case where you aren't showing a button because you're offline, and you're not showing a button because it isn't an available function? If I don't have a connection to the remote, there is no way to tell whether I should or should not be displaying a button! The only other approach I can think of is to create *all* buttons, including ones that the user will never be able to use, and then enable them when connectivity is restored. But as we can't really listen for a "connection restored" event, that's going to require the user to refresh or reopen anyway. Or, you could simply show all buttons and neable them all, but then you're not providing any cueing for the user about what it makes sense to do. This seems to be another case where you think there should be an easy answer but there isn't and perhaps our replication of the web UI is forcing us to mimic solutions that are much more complex than they need to be. This issue never comes up in a Web UI, because you never have the problem of having to support offline access! > of caching data locally is that I don't have to waiting for a repository > connection before triggering an interaction but that seems broken in the current > review editor. Yeah, I see your point but see above; I'm not sure even from a conceptual POV how to improve this.
In trying to come up with a simplification scenario, I've had trouble designing something that makes sense, but this is the closest I can come.. 1) Create all buttons regardless of whether users can use them or we have connectivity. 2) In case where we *don't* have access, enable them all? (Seems backward, but see Steffen's note above about not wanting to force user to refresh when connectivity is restored.) If user tries to connect without having a remote, we simply fail with a no remote connection error. ( We could try to get a remote after the user clicks Submit or whatever.. But we won't really be able to do that since we don't even know what to present to user w/o a connection in many cases and in any case it will be asynchronous, which introduces new complexities.) 3) In cases where we do have access, selectively enable buttons based on user actions. 4. When user refreshes the entire view, the button enablement state will also be refreshed without needing any update logic (I think, since the controls in this case will be rebuilt.)
In the task editor, the submit button is enabled reagardless of connectivity. If there is no connection when the user presses submit, or if the task can't be submitted because of incoming changes or workflow rules, the user gets an error. Should we follow the same principles for the review editor? I.e. create the buttons which make sense based on the state of the review when the editor is opened/refreshed, and if the state changes in the background, we detect an incoming when the user clicks the button and force them to refresh.
(In reply to comment #16) > In the task editor, the submit button is enabled reagardless of connectivity. If > there is no connection when the user presses submit, or if the task can't be > submitted because of incoming changes or workflow rules, the user gets an error. > Should we follow the same principles for the review editor? I.e. create the > buttons which make sense based on the state of the review when the editor is > opened/refreshed, and if the state changes in the background, we detect an > incoming when the user clicks the button and force them to refresh. So that works in some cases, but not in others. We may not even know the proper state until we actually have the remote data. But I'm just going through all of the UI factories. Perhaps we can know this from the review data, which would be a great simplifying assumption, but might also require us to collect more information in the model, and I don't think we should make those kind of changes at this point.
Here are two cases: For Rebase, we need to check the change detail for whether a rebase can occur. There we're getting the information from the change. I have no idea what the rules are there, and I don't think we want an actual value in the model for this (?) as it seems that it could change based on external circumstances. I mean it seems that you can only rebase the latest change, but I'm not sure and wouldn't want to make that assumption. GerritChange change = getChange(); return change != null && change.getChangeDetail() != null && change.getChangeDetail().canRebase(); We have the same issue with restore: GerritChange change = getChange(); return change != null && change.getChangeDetail() != null && change.getChangeDetail().canRebase(); OTOH, for submit, we *may* be able to discover that from the model, as we already are obtaining approvals and placing them in the model: public boolean isExecutable() { ChangeDetailX changeDetail = getChange().getChangeDetail(); if (changeDetail != null) { if (changeDetail.getCurrentActions() != null) { return changeDetail.getCurrentActions().contains(ApprovalCategory.SUBMIT); } else if (changeDetail instanceof ChangeDetailX) { // Gerrit 2.2 and later return changeDetail.canSubmit(); } } return false; } But that's iffy.
One other example of how the complexities exist. Note that because of the asynchronous nature of the call, we can't reliably determine what to do. Consider what happens when a user first opens a review. 1. In the case where we don't have a remote, we'd want to build the entire form with all of the patch set sections and so on, creating the buttons and setting the actual contents from the model when the user expands. 2. In the case where we do have a remote, we want to first build the entire form. *But*, we also request an update, and fill in the details when that update arrives. Note that we can't do that at Review load time for the patch set contents, because those will have new comments that come from PathSetContents. But we can do that for the buttons, because the PatchSetDetails have all of the information we need to do that. So we're faced with a situation where we have to create the buttons, and then recreate them again as soon as the update occurs. Here's the problem: We don't know if we have case 1 or 2 when we build the form! (We don't have the Remote Change the first time around.) This means that we cannot wait for the Review load to complete before we go on to create the buttons. Therefore, the first time we build the buttons, we can't make a good decision. We have to have a separate notification that occurs. But that notification comes from the Review model, *not* the patch set. So we have a situation where you actually have two separate async calls affecting the same controls! (Note that the simplifying assumption above doesn't really fix the complexity, it only makes it less error prone, as we wtill want to update button enablement when the review updates.) Now, given how long the above has taken to explain, imagine how many things can go wrong with this picture. ;D My feeling is that we have to reduce the complexity of this. It's just too delicate. But, let's say we don't update anything at all, we just rely on the current model state when the user opens the editor. Well, that means that for example, we're not doing things like updating to show the current comments, which just seems wrong to me.
Question: Is it the case that you can only abandon and rebase (and perhaps submit) the latest change? That is are these really review level actions as opposed to patch set level? If that's the case that would make a *great* simplifying assumption, and we could even move those buttons to the review section. Since everything else would be patch set specific, that would mean that we could simply build all of those buttons at run time and leave them. We could even do something nice like move them to the section header (I know, not for 2.0 :)).
(In reply to comment #14) > (In reply to comment #12) > > Does that mean users have to press the refresh button in order to get the > > changes for a review regardless of task synchronizations (happening in the > > background)? > > Yes, which feels kind of weird and sub-optimal to me given that we can easily > get that data, but seems consistent with the way other connectors handle > "non-partial" data. No, that is not correct. Other connectors retrieve partial data when the query runs and retrieve the full data in a separate task synchronization that runs for all tasks that were identified as having changed. No properly implemented connector that I know of *requires* explicit user interaction to fully synchronize tasks. The current Gerrit connector refresh design seems very broken to me. > This seems to be another case where you think there should be an easy answer but > there isn't and perhaps our replication of the web UI is forcing us to mimic > solutions that are much more complex than they need to be. I don't get why this is complicated. It's equivalent to the workflow state in the task editor: The valid operations are cached in task data. This was also the case for the patch set workflow in 1.0. I don't see any reason why this couldn't be cached in the reviews model in the same way.
Ignore that last Q.. these are in fact all patch set specific according to API, even though Abandon is an action that makes sense only for the review as a whole. (In reply to comment #21) > No, that is not correct. Other connectors retrieve partial data when the query > runs and retrieve the full data in a separate task synchronization that runs for > all tasks that were identified as having changed. No properly implemented > connector that I know of *requires* explicit user interaction to > fully synchronize tasks. The current Gerrit connector refresh design seems very > broken to me. My mistake -- I didn't think that through properly. I think this way because there are so many occasions where I do have to refresh task content manually, because a task editor is already open. Note for the Gerrit Connector though, somewhere along the line I *believe* that we did make the decision that it wasn't appropriate to grab the model for every open review task, IIRC because we weren't storing the data not in TaskData but in the model persistence layer. But I'm more than happy to do it this way instead. However..that's almost a moot issue, as I know that we decided that we would not do this for patch sets, because of the number of patch set access we would need to make. Didn't we decide to do this in background, and isn't that consistent w/ original behaviour? > > This seems to be another case where you think there should be an easy answer > but > > there isn't and perhaps our replication of the web UI is forcing us to mimic > > solutions that are much more complex than they need to be. > > I don't get why this is complicated. It's equivalent to the workflow state in > the task editor: The valid operations are cached in task data. But they can't be in this case, because they're patch set specific. The 1.0 implementation was determining this at build time. The difference is that the 1.1 connector was using the neat trick of reconstitituing the Gerrit objects from the task data. That's something that for better or worse we aren't going to be doing anymore. > This was also the > case for the patch set workflow in 1.0. I don't see any reason why this couldn't > be cached in the reviews model in the same way. Well, all of these issues arose essentially because I thought that we couldn't make this assumption and that Patch Set contents have to be updated based on explicit user interactions. Given that, that drives how we obtain Patch Sets, but design wise it also (quite arguably) called for a more model driven approach to the updates from remote in general, as otherwise we're maintaining two separate data/remote change notification approaches. But it turns out we're having to do that anyway, because we need to work with Editor life-cycle. Again, I see the issue as not that either approach is broken -- they both work very well in isolation -- but that they don't play well together.
I think you can rebase any change, even though the web UI only shows the rebase button on the latest. But one can already get into a state where buttons are enabled that don't work, e.g. when trying to publish comments on what is no longer the latest patch set, you get an error that the change is closed. (In reply to comment #19) > complexity of this. It's just too delicate. But, let's say we don't update > anything at all, we just rely on the current model state when the user opens the > editor. Well, that means that for example, we're not doing things like updating > to show the current comments, which just seems wrong to me. When opening the task editor, a background sync happens and the user is prompted to refresh the editor if anything has changed. Why wouldn't we do the same thing for Gerrit reviews, with the background sync pulling down whatever info is needed to display the right buttons and so on? I could see an argument that Gerrit reviews change so often that we want some more realtime sync to happen, but that seems like it should be addressed on the framework level rather than in this specific connector, e.g. by supporting "progressive" rendering of the task editor, where sections are created one at a time as the information is available, and allowing the editor to be refreshed without blowing away the state of the controls (expansion, etc.). The Gerrit connector seems to have an awkward mixture of this and the traditional approach.
(In reply to comment #23) > I think you can rebase any change, even though the web UI only shows the rebase > button on the latest. But one can already get into a state where buttons are > enabled that don't work, e.g. when trying to publish comments on what is no > longer the latest patch set, you get an error that the change is closed. True enough. It would be nice to provide some cueing though so users don't think they can do all of these things and they can't. As a first-order though, I think we should just create them and handle whatever comes up on the backend. > (In reply to comment #19) > When opening the task editor, a background sync happens and the user is prompted > to refresh the editor if anything has changed. Why wouldn't we do the same thing > for Gerrit reviews, with the background sync pulling down whatever info is > needed to display the right buttons and so on? That's exactly what we do, except that the model update isn't happening in the background Sync. This seems to work reasonably well. I think I may have some good approaches to simplifying the update issues after taking a fresh look yesterday. > I could see an argument that Gerrit reviews change so often that we want some > more realtime sync to happen, but that seems like it should be addressed on the > framework level rather than in this specific connector, e.g. by supporting > "progressive" rendering of the task editor, where sections are created one at a > time as the information is available, and allowing the editor to be refreshed > without blowing away the state of the controls (expansion, etc.). The Gerrit > connector seems to have an awkward mixture of this and the traditional approach. Yeah, it is incredibly awkward, but the whole point of this project has been as a pilot for a more model-based / progressive approach but not to actually implement that for Mylyn tasks as a whole. Framework level changes were really way out of scope.
Alright, thanks Steffen and Sam. Your comments have been extremely helpful in clarifying things. So I've been experimenting with moving the update life-cycle into the Task Data update mechanism proper and so far so good, but I am concerned about making such a significant change in how things are set up. I'm hoping to have a preliminary review up later today.
Comments about https://git.eclipse.org/r/#/c/13154/7: * The Restore button is shown even though Restore is not a valid action * When Abandon or Restore are used and the dialog is cancelled it shows again, it seems that all actions are executed twice * If I restore an abandoned review the editor refreshes but the status is still ABANDONED (the web ui shows it as restored) * When a review changes and I refresh the query and incoming link is shown. Pressing the link has no effect. I have to reopen the editor to see the changes. * If I change the URL of the Gerrit repository to something invalid, restart and refresh the task all data disappears from the editor and task list for the task. No error is displayed but several exceptions are logged. * When I manually refreshed the task the job never finished. It was trying to fetch 5 patch sets while only 4 completed. It was looping forever around GerritTaskDataHandler:151. Canceling didn't have any effect. This was the result of 20 min. of testing. Miles, how do you suggest we proceed? It doesn't even look like the automated tests are passing.
Steffen, that was just a WIP push to keep things relatvily up to date. I've changed the way we're doing everything and I wanted you to be able to see where that's headed in code. I'm going to be pushing a set of changes in next hour or two that should be a lot closer to where we need to be functionally.
https://git.eclipse.org/r/#/c/13154/8 This one is a much more solid cut at task handler approach -- which in general seems to work much better -- but expect significant issues still. I'm not sure why the Sync tests are failing -- they pass when I run them directly, e.g. launching a local JUnit Test on gerritSynchronizationTest, but fail when I launch AllGerritTests with -Dorg.eclipse.mylyn.tests.all=true. If we're able to get this working reliably, I'm going to look into simplifying the code -- perhaps ripping out the RemoteEmfClient stuff -- then reverting https://git.eclipse.org/r/#/c/12229/ and submitting it in more granular reviews.
https://git.eclipse.org/r/#/c/13154/9 is much more functional. Review Explorer is working again. Right now, I'm working on backing out the changes in commit 85c80d97bc4e2f9bcc6df383eceaf56ede459d0c. This way we can replay with more bite sized changes. See details below: Author: Miles Parker <milesparker@gmail.com> 2013-05-01 20:33:22 Committer: Miles Parker <milesparker@gmail.com> 2013-05-21 20:52:51 Parent: d7bec6281ddafafc56a3b2e366a2d7232bfd84a8 (395059: update test repositories to Gerrit 2.5.4, 2.6-rc3, 2.7-rc1) Child: 96a0e79969a85b6593c67b5bf78d70c740c3d6f0 (408737: [regression] RuntimeException (when opening multiple reviews?)) Branches: bug389944PatchSetCaching, bug409343LifeCycle, bugg409343NoProxies, master, rewind, origin/e_4_3_m_3_9_x, origin/master 394020: [model] Persist Review models across workbench.. •Adds Edit Factory Supporting Persistence •Enriches Consumer and Observers to support complex remote requests •Refactors Editors, Explorer and UI factories to implement above •Ensure that model changes don't happen off of model thread •Adds testing coverage
Forgot the reversion review link: https://git.eclipse.org/r/#/c/13631/
I tried https://git.eclipse.org/r/#/c/13154/9: * Rebase button is disabled for all patch sets * Comment section kept collapsing by itself - can't reproduce this now * Lots of errors about "Parent and local keys must be specified"
It also seemed like this forced a refresh of all my reviews.
> I tried https://git.eclipse.org/r/#/c/13154/9: > > * Rebase button is disabled for all patch sets > * Comment section kept collapsing by itself - can't reproduce this now > * Lots of errors about "Parent and local keys must be specified" Okay, hmm.. I'm not seeing any of these things. Please: 1. Go up against Eclipse Bugzilla Reviews (Just so we have a baseline to compare) -- I'm using mylyn/org.eclipse.mylyn.context.mft or reviews because they're relatively small sets. 1. Delete current queries and tasks (after they got to Uncategorized) 2. Exit 3. Do: rm -rf {workspace_location}/.metadata/.mylyn/model 4. Restart
I just tried all of this in Windows, still unable to reproduce. Hopefully the steps above will sort it out. (In reply to comment #32) > It also seemed like this forced a refresh of all my reviews. Yes, the first time you open the reviews, you have to manually refresh. Should be able to fix that issues easily.
I'm stuck on the GerritSyncronizationTests. These are running fine stand-alone, but break when they're run as AllGerritTests w/ -Dorg.eclipse.mylyn.tests.all=true. I think there is an exception that might be getting buried here. When I try to create the client, I get: Thread [Worker-0] (Suspended (exception NullPointerException)) ConcurrentHashMap<K,V>.get(Object) line: 922 GerritConnector.loadConfiguration(TaskRepository) line: 346 GerritConnector.createReviewClient(TaskRepository, boolean) line: 327 These are the lines: protected GerritConfiguration loadConfiguration(TaskRepository repository) { GerritConfiguration configuration = configurationCache.get(repository); I think there is some expectation of the harness that with these changes I'm not meeting. Steffen, any ideas about what might be going on here?
I tested with patch set 14 and I'm still missing buttons when expanding older patch sets. Sometimes they appear but most often not. When I added a patch set through the console the refresh link on top of the task editor that appeared didn't work (nothing seemed to happen). Surprisingly the new patch set showed up in the Compare With drop down though but resulting in this exception when selected: !MESSAGE Unhandled event loop exception !STACK 0 java.lang.NullPointerException at org.eclipse.mylyn.internal.gerrit.ui.factories.CompareWithUiFactory.execute(CompareWithUiFactory.java:151) at org.eclipse.mylyn.internal.gerrit.ui.factories.CompareWithUiFactory$3$1.widgetSelected(CompareWithUiFactory.java:132) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:240) After adding a comment and then using Compare with Base I got this exception: !MESSAGE An internal error occurred during: "Retrieving Compare Patch Set 1 with Base". !STACK 0 java.lang.NullPointerException at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentRemoteFactory.pull(PatchSetContentRemoteFactory.java:80) at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentCompareRemoteFactory.pull(PatchSetContentCompareRemoteFactory.java:38) at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentCompareRemoteFactory.pull(PatchSetContentCompareRemoteFactory.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) Initially when I tried to the connector everything failed with an "Problem performing query: OK" or "Problem updating repository: OK". I traced it to the login which was returning HTTP OK (200). Not sure if that's related to the changes or a problem that has been around for a while. A restart eventually fixed it. I noticed that when I added a comment through the compare editor the comment count in the patch set header was updated (e.g. said 2 Comments) which is a bit confusing since the new comment was only a draft at that point. The decorations in the file list were not refreshed, only when the editor was reopened. It's minor but something that we should track to fix it later.
(In reply to comment #35) > I'm stuck on the GerritSyncronizationTests. These are running fine stand-alone, > but break when they're run as AllGerritTests w/ > -Dorg.eclipse.mylyn.tests.all=true. I think there is an exception that might be > getting buried here. If I run AllGerritTests I get this dumped to the console (haven't investigated further whether it's related to the test failures): !ENTRY org.eclipse.mylyn.tasks.core[2013-06-10T4-50-33] Status ERROR: org.eclipse.mylyn.tasks.core code=0 Synchronization failed java.lang.NullPointerException, Exception: java.lang.NullPointerException at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:768) at org.eclipse.mylyn.internal.gerrit.core.GerritConnector.loadConfiguration(GerritConnector.java:346) at org.eclipse.mylyn.internal.gerrit.core.GerritConnector.createReviewClient(GerritConnector.java:327) at org.eclipse.mylyn.reviews.core.spi.ReviewsConnector.getReviewClient(ReviewsConnector.java:30) at org.eclipse.mylyn.internal.gerrit.core.GerritConnector.getClient(GerritConnector.java:144) at org.eclipse.mylyn.internal.gerrit.core.GerritConnector.hasTaskChanged(GerritConnector.java:223) at org.eclipse.mylyn.internal.tasks.core.data.TaskDataManager.putUpdatedTaskData(TaskDataManager.java:195) at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.updateFromTaskData(SynchronizeTasksJob.java:312) at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.synchronizeTask(SynchronizeTasksJob.java:247) at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.runInternal(SynchronizeTasksJob.java:218) at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:153) at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:135) at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeQueriesJob.run(SynchronizeQueriesJob.java:225) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Please note btw that the patch sets I'll be updating while diagnosing the testing issue aren't necessarily the best current implementation. In some cases I've modified the code in subtle ways to try to determine what is causing issues in the test configuration. I've been noting that in commit comment when I post an update ("DIAGNOSE TESTING ISSUES"). (In reply to comment #36) > I tested with patch set 14 and I'm still missing buttons when expanding older > patch sets. Sometimes they appear but most often not. Yep, I'm seeing that as well. > When I added a patch set through the console the refresh link on top of the task > editor that appeared didn't work (nothing seemed to happen). I'll take a look. I'm not actually sure btw what policy to use to grab patch set details in the Task Update. I'd actually wrtitten code that could grab all patch sets, but I'm not sure if that's a good idea. It can take a long time to get a single review, and users of often won't be interested in that. At the same time, it did sort of seem to make sense to grab the last one retrieved, and perhaps to update all of those patch sets that had *previously* been requested by the user. For now, I've removed all of the patch set update code as I try to simplify things to test. > Surprisingly the > new patch set showed up in the Compare With drop down though but resulting in > this exception when selected: > !MESSAGE Unhandled event loop exception > !STACK 0 > java.lang.NullPointerException > at > org.eclipse.mylyn.internal.gerrit.ui.factories.CompareWithUiFactory.execute(CompareWithUiFactory.java:151) Right, that makes sense, as we don't have the patch set there yet. I'm going to look at some further simplification of this code. > I noticed that when I added a comment through the compare editor the comment > count in the patch set header was updated (e.g. said 2 Comments) which is a bit > confusing since the new comment was only a draft at that point. The decorations > in the file list were not refreshed, only when the editor was reopened. It's > minor but something that we should track to fix it later. Ok, I'll keep that one in mind.
(In reply to comment #37) > If I run AllGerritTests I get this dumped to the console (haven't investigated > further whether it's related to the test failures): Yeah, I've been seeing this too. What the heck is going on here? The code calling order is basically exactly the same. I'll try to isolate the change I made here from the rest of the code. > > !ENTRY org.eclipse.mylyn.tasks.core[2013-06-10T4-50-33] Status ERROR: > org.eclipse.mylyn.tasks.core code=0 Synchronization failed > java.lang.NullPointerException, Exception: > java.lang.NullPointerException > at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:768) > at > org.eclipse.mylyn.internal.gerrit.core.GerritConnector.loadConfiguration(GerritConnector.java:346) > at > org.eclipse.mylyn.internal.gerrit.core.GerritConnector.createReviewClient(GerritConnector.java:327) > at > org.eclipse.mylyn.reviews.core.spi.ReviewsConnector.getReviewClient(ReviewsConnector.java:30) > at > org.eclipse.mylyn.internal.gerrit.core.GerritConnector.getClient(GerritConnector.java:144) > at > org.eclipse.mylyn.internal.gerrit.core.GerritConnector.hasTaskChanged(GerritConnector.java:223) > at > org.eclipse.mylyn.internal.tasks.core.data.TaskDataManager.putUpdatedTaskData(TaskDataManager.java:195) > at > org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.updateFromTaskData(SynchronizeTasksJob.java:312) > at > org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.synchronizeTask(SynchronizeTasksJob.java:247) > at > org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.runInternal(SynchronizeTasksJob.java:218) > at > org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:153) > at > org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:135) > at > org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeQueriesJob.run(SynchronizeQueriesJob.java:225) > at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
The reason tests are failing is that task synchronization now relies on stuff being executed on the UI thread: Thread [Worker-5] (Suspended) waiting for: Semaphore (id=160) Object.wait(long) line: not available [native method] Semaphore.acquire(long) line: 43 UISynchronizer.syncExec(Runnable) line: 168 Display.syncExec(Runnable) line: 4613 RemoteUiService.modelExec(Runnable, boolean) line: 35 GerritRemoteFactoryProvider(AbstractRemoteFactoryProvider).modelExec(Runnable, boolean) line: 28 GerritRemoteFactoryProvider(AbstractRemoteEditFactoryProvider<ERootObject,EChildObject>).modelExec(Runnable, boolean) line: 315 GerritRemoteFactoryProvider.pullUser(IRepository, AccountInfoCache, Account$Id, IProgressMonitor) line: 72 GerritReviewRemoteFactory.pull(IRepository, String, IProgressMonitor) line: 91 GerritReviewRemoteFactory.pull(EObject, Object, IProgressMonitor) line: 1 RemoteEmfConsumer<EParentObjectType,EObjectType,LocalKeyType,RemoteType,RemoteKeyType,ObjectCurrentType>.pull(boolean, IProgressMonitor) line: 164 RemoteUiService(JobRemoteService).retrieve(AbstractRemoteConsumer, boolean) line: 93 RemoteEmfConsumer<EParentObjectType,EObjectType,LocalKeyType,RemoteType,RemoteKeyType,ObjectCurrentType>.retrieve(boolean) line: 289 GerritTaskDataHandler.updateModelData(TaskRepository, TaskData, ReviewObserver, IProgressMonitor) line: 146 GerritTaskDataHandler.getTaskData(TaskRepository, String, IProgressMonitor) line: 114 GerritConnector.getTaskData(TaskRepository, String, IProgressMonitor) line: 160 SynchronizeTasksJob.synchronizeTask(IProgressMonitor, ITask) line: 245 SynchronizeTasksJob.runInternal(Set<ITask>, IProgressMonitor) line: 218 SynchronizeTasksJob.run(Set<ITask>, IProgressMonitor) line: 153 SynchronizeTasksJob.run(IProgressMonitor) line: 129 Worker.run() line: 54 This doesn't work for the current test design (since the test runs on the UI thread hence the event loop isn't spun and the executables run at the "wrong" time).
Yes, that was one of the things I I was going to ask you about that. I had thought about changing this in any case, but it does introduce new (different) risks. Is this an issue with testing or with framework requirements themselves? In other words, should all well-designed connector implementations never use the UI thread, or is this just an incidental assumption of the framework? I've isolated another case of the testing issues to the following review though. I'm not sure this is the same UI issue. https://git.eclipse.org/r/#/c/13710/
(In reply to comment #40) > This doesn't work for the current test design (since the test runs on the UI > thread hence the event loop isn't spun and the executables run at the "wrong" > time). And makes sense, because it explains why they *do* work when launched from a local unit test. I'm going to look at moving these changes to a non-UI thread. It would be cleaner on some sense, but then we have to be really careful about isolating these changes against changes from the editor, which was one of the complexities I was hoping to avoid with this design. I may be able to make a clean separation based on function here though. Question: When a user asks for a refresh, is the editor completely locked down during the refresh?
(In reply to comment #42) > Question: When a user asks for a refresh, is the editor completely locked down > during the refresh? The controls are disabled but you can do things like closing that editor instance and reopening it (which gets you an editor with enabled controls). Users can also trigger task synchronizations directly from the task list at any time.
(In reply to comment #43) > (In reply to comment #42) > > Question: When a user asks for a refresh, is the editor completely locked down > > during the refresh? > > The controls are disabled but you can do things like closing that editor > instance and reopening it (which gets you an editor with enabled controls). > Users can also trigger task synchronizations directly from the task list at any > time. Yeah, it's really not something to rely on for model access control. I think the basic policy of forcing discrete changes to occur on UI is still by far the most robust approach. Even thought there isn't that much that users can change, and we could probably work out a decent isolation if needed to, that would require all UI elements to check out and notify for access against Task Sync, and there is just a lot of stuff to maintain there that isn't really neccesary if we take the "get in and get out quickly" approach to model updating. Note that because all of the reviews are actually seperate file resources, we could imagine a future scenario where we might support some kind of bulk update locking at the file/review level.
(In reply to comment #36) > I tested with patch set 14 and I'm still missing buttons when expanding older > patch sets. Sometimes they appear but most often not. Actually, it turns out that they were actually there. They just weren't getting displayed because of a layout issue. :)
And then there were 2. ;) I don't want to close these yet as they both report issues that occurred recently and I haven't had confirmation on the fixes for.
While testing master I found bug 410486, not sure if this is a regression, but looks like one.
It doesn't look SynchronizeTasksJob properly guards against runtime exceptions from connectors so bug 410486 could break synchronization leading to missed updates (even for other tasks that changed).
The majority of the issues has been addressed. Let's close this task and track the remaining work on new bugs. Thanks a lot for getting this into shape so close to the release, Miles!
(In reply to comment #49) > The majority of the issues has been addressed. Let's close this task and track > the remaining work on new bugs. Thanks a lot for getting this into shape so > close to the release, Miles! Next time I'd like to do it as far away from the release as possible! :D
