Bugzilla – Full Text Bug Listing
| Summary: | stop automatic configuration retrieval if already done | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Robert Elves <robert.elves> | ||||||||||||||||||||||||||||
| Component: | Mylyn | Assignee: | maarten meijer <mjmeijer> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||||||||||
| Priority: | P2 | CC: | mjmeijer, steffen.pingel | ||||||||||||||||||||||||||||
| Version: | unspecified | ||||||||||||||||||||||||||||||
| Target Milestone: | 2.2 | ||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Robert Elves
Is this the same as 207660: do not update repository configuration on every startup? *** Bug 207660 has been marked as a duplicate of this bug. *** It should be considered to save the time stamp for each repository. Currently manual synchronizations are not taken into account when calculating the interval but this could also help to reduce the number of automatic background synchronizations. Marten, for clerical purposes, do you mind posting your patch here? Thanks. Reassigning to you. (In reply to comment #3) > It should be considered to save the time stamp for each repository. Currently > manual synchronizations are not taken into account when calculating the interval > but this could also help to reduce the number of automatic background > synchronizations. This would be a nice additional optimization. Created attachment 81642 [details]
Patch to store last config update day to repository property store
Also includes touch() method to be called when doing manual updates.
Created attachment 81643 [details]
mylyn/context/zip
Created attachment 81647 [details]
Mylyn Repository Spy
A simple view (very rough) to look at TaskRepository properties.
This shows whether the property is actually persisted.
(In reply to comment #3) > It should be considered to save the time stamp for each repository. Done > Currently manual synchronizations are not taken into account when calculating the interval > but this could also help to reduce the number of automatic background > synchronizations. Provided static method to be called when doing manual update. Will search right spot once this is seen as the route forward. (In reply to comment #7) > Created an attachment (id=81647) > Mylyn Repository Spy > A simple view (very rough) to look at TaskRepository properties. > This shows whether the property is actually persisted. I like that, but instead of custom view it should link standard Properties with task repositories and other Mylyn artifacts, such as queries or tasks (In reply to comment #9) > I like that, but instead of custom view it should link standard Properties with > task repositories and other Mylyn artifacts, such as queries or tasks Also nice and possible, but I think we should not bother Users with the stuff we Developers want. I have Metadata Tools, Resources Tools and Runtime Tools as developer with loads of spies to see whether you added the property correctly, what the delta in your builder looks like, etc. What you want is a new bug: Task, Category and Repository should fill the standard Properties view: http://dev.eclipse.org/blogs/wayne/2007/10/08/getting-started-with-properties/ http://dev.eclipse.org/blogs/wayne/2007/10/15/adapters/ http://dev.eclipse.org/blogs/wayne/2007/10/23/adapters-part-deux/ (In reply to comment #10) > > I like that, but instead of custom view it should link standard Properties > with > What you want is a new bug: Task, Category and Repository should fill the > standard Properties view: > http://dev.eclipse.org/blogs/wayne/2007/10/08/getting-started-with-properties/ > http://dev.eclipse.org/blogs/wayne/2007/10/15/adapters/ > http://dev.eclipse.org/blogs/wayne/2007/10/23/adapters-part-deux/ Created bug 208275: Task, Category and Repository should fill the standard Properties view: https://bugs.eclipse.org/bugs/show_bug.cgi?id=208275 The bugzilla connector (the main one retrieving contexts) has a property called org.eclipse.mylyn.tasklist.repositories.config.timestamp that stores config update information. Are there independent streams trying to solve the same problem? All this is revealed with my Repository Spy View (see attachment) Created attachment 81890 [details]
Version with proper icons/actions
Should allow one to clear any property
Looking good, just a few nits:
* Lets store the configuration update tiem on TaskRepository with set/getConfigurationTimestamp().
* Use regular long timestamp and subtract it from now to see if a day has passed
* Consider moving:
// only persist if actually changed
if (lastRepositoryRefresh == null) {
lastRepositoryRefresh = Calendar.getInstance();
saveLastRefreshToStore(repository, lastRepositoryRefresh);
}
...up into the above clause since that runs the job I believe (only if the job runs is the config being updated).
(In reply to comment #12)
> The bugzilla connector (the main one retrieving contexts) has a property called
> org.eclipse.mylyn.tasklist.repositories.config.timestamp that stores config
> update information.
> Are there independent streams trying to solve the same problem? All this is
> revealed with my Repository Spy View (see attachment)
This is used to store the timestamp of the actual repository configuration (if available). I should idally have used a slightly different id perhaps. Most connectors (and bugzilla installs) will not support this.
(In reply to comment #14) > // only persist if actually changed > if (lastRepositoryRefresh == null) { > lastRepositoryRefresh = Calendar.getInstance(); > saveLastRefreshToStore(repository, lastRepositoryRefresh); > } > > ...up into the above clause since that runs the job I believe (only if the job > runs is the config being updated). Yes, that will handle start condition w/o property as well. > (In reply to comment #12) > This is used to store the timestamp of the actual repository configuration (if > available). I should idally have used a slightly different id perhaps. Most > connectors (and bugzilla installs) will not support this. I also see a bunch of org.eclipse.mylar.... properties. these are from before the name change :-) Maybe my spy should allow cleaning these up, or there can be some cleanup code to get rid of outdated config stuff like this on initial run of 2.2.0. I will use setConfigurationTimestamp() to set the times tamp also when doing a manual update as opposed to scheduled. Perfect! If you could contribute a unit test that ensures the logic to retrieve configuration is valid, that would be great as well. Creating the unit tests raised some more questions: - I can add a setConfigurationTimepstamp() and getConfigurationTimepstamp() to TaskRepository. When they use Calendars as parameter and return values, the date logic becomes spread When they use Strings as parameter and return values, the date logic becomes complicated wiith locale issues, etc I now the Jira stuff has its own datToString() and stringToDate logic, so uses Date() as time representation. There are other bugs with Date/Timestamp issues as well. Before proceeding we should standardize our Date/Time representation across the whole of Mylyn Why not hide logic in TaskRepository with a simple isConfigRefreshNeeded(); I think not even a manual update should take place when one was done less than 2 minutes ago. (In reply to comment #17) > Creating the unit tests raised some more questions: > - I can add a setConfigurationTimepstamp() and getConfigurationTimepstamp() to > TaskRepository. Couldn't we pass in and receive a Date object for both? When getConfigurationTimestamp() returns a Date, we can get the long time value and just check if the current date is 86400000 milliseconds past (24hrs). TaskRepository methods abstract how the date value is persisted (unix long would be best in this case). > When they use Calendars as parameter and return values, the date logic becomes > spread > When they use Strings as parameter and return values, the date logic becomes > complicated wiith locale issues, etc Since this timestamp is only interpreted locally, the above use of Date should work. > There are other bugs with Date/Timestamp issues as well. Before proceeding we > should standardize our Date/Time representation across the whole of Mylyn > Why not hide logic in TaskRepository with a simple isConfigRefreshNeeded(); I As a baby step we could eliminate this doubling up configuration timestamp checking at least (that you've observed). We could encapsulate this all within an AbstractTaskRepository.updateAttributes(TaskRepository repository, boolean force, IProgressMonitor monitor) method which in turn delegates to the existing updateAttributes and isRepositoryConfigurationStale methods as needed. > think not even a manual update should take place when one was done less than 2 > minutes ago. Manual update needs to take action if a user requests it since optimization of this kind can cause confusion (i.e. the user has changed some attributes on the repository many times in a row, synching each time, expecting to see the changes in the task editor) (hence the need for 'force' parameter in the method above) (In reply to comment #18) > As a baby step we could eliminate this doubling up configuration timestamp > checking at least (that you've observed). We could encapsulate this all within > an AbstractTaskRepository.updateAttributes(TaskRepository repository, boolean > force, IProgressMonitor monitor) method which in turn delegates to the existing > updateAttributes and isRepositoryConfigurationStale methods as needed. I cannot get at the connector: final AbstractRepositoryConnector connector = TasksUiPlugin.getRepositoryManager().getRepositoryConnector(repository.getConnectorKind()); is in TaskUIPlugin, and TaskRepository is in core. This would introduce a cyclic dependency :-( Or should the rep[ository manager move to core? Created attachment 82010 [details]
mylyn/context/zip
Moved logic to RepositoryConnector
Added touch() on Bugzilla manual updates
Created attachment 82011 [details]
Resolved nits, using Date()
Created attachment 82012 [details]
mylyn/context/zip
Rob: please review. I had a look but the patch seems to have gone stale and some classes are missing. Could you repost Maarten? Some additional nits: Date comparison could be done in TaskActivityUtil. Also consider just storing a simple long (if not already). Instead of touchConfigurationTimestamp, although this is familiar unix language, we should consider calling it updateConfigurationTimestamp(). Created attachment 82280 [details] New patch in response to comment 24 - renamed touch...() to update...() - moved Date stuff to TaskActivityUtil - kept Dates as strings rather than longs for my repository spy view in sandbox :-) Created attachment 82281 [details]
mylyn/context/zip
Created attachment 82408 [details] Updated patch We're getting there. Here is an updated version of your patch that takes into consideration your observations from comment#12. In this scenario the connectors default to a 24hr refresh (as we intend) and this minimizes the api additions to TaskRepository. Oh and we'll need to tag those methods with "@Since 2.2". Have a look and see what you think of this Maarten. Created attachment 82409 [details]
mylyn/context/zip
I like this! The was a mix of ...ConfigurationDate() and ...ConfigurationTimestamp() but that was easy to fix. Needed some more javadoc clean up: TimestampUtils is gone again, so I'll do that But most important, in ScheduledTaskListSynchJob.run() we PREFLIGHT whether to create a job with repository.configUpdateNeeded() and IN the Job test again to do any real work with connector.isRepositoryConfigurationStale(repository) I think we need preflight with isRepositoryConfigurationStale() so we can drop configUpdateNeeded() for less API change. Then isRepositoryConfigurationStale() throws CoreException, is this needed? Why not catch, log and return true or false on error, as with malformed url exception in BugzillaRepositoryConnector @override And last doesn't default isRepositoryConfigurationStale() need to check for offline and return false (can't check) if so? (In reply to comment #29) > I like this! > The was a mix of ...ConfigurationDate() and ...ConfigurationTimestamp() but that > was easy to fix. > Needed some more javadoc clean up: TimestampUtils is gone again, so I'll do that Is it possible your workspace wasn't clean before applying the patch to your workspace? This and the other api cleanup you mention are part of the patch. > Then isRepositoryConfigurationStale() throws CoreException, is this needed? > Why not catch, log and return true or false on error, as with malformed url > exception in BugzillaRepositoryConnector @override CoreException is thrown here to allow for error presentation/recovery in the event of connector failure (i.e. communication with repository error). > And last doesn't default isRepositoryConfigurationStale() need to check for > offline and return false (can't check) if so? Yes ideally the connector's implementation would do this when overriding the isRepositoryConfigurationStale() method. Created attachment 82502 [details]
Merged patch
I think the config update should be set to last successful update. I changed javadoc to make this explicit
So it must not be set in isRepositoryConfigurationStale() but explicitly set after updating: scheduled or manual.
Added javadoc with suggestions to implementors.
I removed isConfigUpdateNeeded as PREFLIGHT testing is now done with isRepositoryConfigurationStale().
I added some Tracing instrumentation to show the actual updates performed on the console: I thought it useful for connector implementors trying to get their logic right, also in combination with the config.update clearing in the repository spy.
In ScheduledTaskListSynchJob I moved the Config update to a separate private method. This method is now only called when the PREFLIGHTING indicates an update is needed, so no UI feedback is presented.
Previously do nothing jobs could be scheduled. I changed the order of the preflight so the isStale test is only performed if any queries are present for that repository. No queries, no config update.
Does this warrant user feedback like "Manual Config update recommended" for the manual config update button in the new query creation wizard? Say when: isStale returns true?
Updated the test code as well to use new names. When OK please add me to the authors :-)
Created attachment 82503 [details]
mylyn/context/zip
(In reply to comment #31) > I think the config update should be set to last successful update. I changed > javadoc to make this explicit In the case of the Apache JIRA for instance this would be dangerous without additional safe-guards. In the current state the update fails so it would be retried on each background synchronization. I am in favor of not attempting to retry the update for 24 hours if it fails (consider logging a warning or info message to the error log). > I added some Tracing instrumentation to show the actual updates performed on the > console: I thought it useful for connector implementors trying to get their > logic right, also in combination with the config.update clearing in the > repository spy. I am not sure logging is required here. It sounds like the API contract needs to be clearly documented in JavaDoc to enable integrators to implement it. Generally we should define a policy for adding this type of logging before we start cluttering the code with logging statements. We also need to make sure that this does not break the head-less build (in this case it is fine I think since tasks.ui is not part of it). Regarding using the console for output, please see http://wiki.eclipse.org/Mylyn_Contributor_Reference#Patches . (In reply to comment #33) > (In reply to comment #31) > > I think the config update should be set to last successful update. I changed > > javadoc to make this explicit > > In the case of the Apache JIRA for instance this would be dangerous without > additional safe-guards. In the current state the update fails so it would be > retried on each background synchronization. I am in favor of not attempting to > retry the update for 24 hours if it fails (consider logging a warning or info > message to the error log). I have a very strong view on this: A is...() method that returns a boolean result should NOT have any side effects, like updating a time stamp. It's like a HTTP GET request, it should not have side effects! During the development I called it twice in a row and then the repository never got updated, because it changed the date before updating. That only took me two hours to find :-( If we want this behavior the method name should be changed to something other than isRepositortyConfigurationStale because that should return true until updated! I propose to add a second overrideable method like isPeriodicConfigUpdateNeeded() to be used in preflightingh config update job creation. It will return true everytime on default implementation and once every 24 hours for JIRA. > > > I added some Tracing instrumentation... > I am not sure logging is required here. It sounds like the API contract needs > to be clearly documented in JavaDoc to enable integrators to implement it. > > Generally we should define a policy for adding this type of logging before we > start cluttering the code with logging statements. We also need to make sure > that this does not break the head-less build (in this case it is fine I think > since tasks.ui is not part of it). Regarding using the console for output, > please see http://wiki.eclipse.org/Mylyn_Contributor_Reference#Patches . I fully agree but... I noticed there already was tracing for HttpClient lib that gets sent to the console. When instrumentation in the base classes is properly set up for development use cases, it will save connector implementors a lot of time, since they do not need to step throught the base class source as much. There are many [connector] marked bugs out there and there exists little help other than te source code. Maybe raise two more bugs: - improve repository connector developers support (mostly in mylyn.tasks) - improve bridge connector developers support (mostly in mylyn.contexts) Maarten, your patch has been applied with minor alterations. Ip log updated. Marking resolved. In the future we will eliminate the multiple calls to connector.updateAttributes() and make a single point within the framework where this is handled at which point the confg date will also be updated. Steffen, note that we now are not setting the time stamp within the isRepositoryConfigurationStale() and explicitly setting this in the ScheduledTaskListSynchJob. (In reply to comment #35) > Steffen, note that we now are not setting the time stamp within the > isRepositoryConfigurationStale() and explicitly setting this in the > ScheduledTaskListSynchJob. It think it should also be set after the manual update, otherwise a scheduled update can occur right after that. What do you think? Second, storing the date as a long makes it more difficult to understand using the Properties View, bug 208275 and bug 210639. (In reply to comment #36) > It think it should also be set after the manual update, otherwise a scheduled > update can occur right after that. Yes, connectors are responsible to change their implementation to set the time stamp after an update. > Second, storing the date as a long makes it more difficult to understand using > the Properties View, bug 208275 and bug 210639. This should be handled by the properties view. Key/values stored in the TaskRepository class can be connector specific and have any format. |