search

vazexqi / CodingSpectator

Watches and analyzes code edits in the Eclipse IDE non-invasively
http://codingspectator.cs.illinois.edu
Other
20 stars 14 forks source link

Evaluate committing data into fresh locations #311

Closed reprogrammer closed 13 years ago

reprogrammer commented 13 years ago

We ran into tricky problems with the data submission mechanism of CodingSpectator while investigating issues #257 and #309. In summary, when a user restores his/her workspace to an older version, Subversion gets confused. When the user's workspace is old, CodingSpectator has to update it to the latest revision to be able to commit it. But, the update operation might lead to some conflicts that are difficult to resolve (See issue #309) or the update operation itself might be undesirable because we'd like to capture the user's workspace as it is (See the comment on issue #257). Therefore, we're exploring alternative ways of handling outdated workspaces. An alternative is to commit the data to a fresh location when the watched folder is outdated. Such commits would hopefully be rare, since they result in potentially huge data transfers. We need to think about the following questions in order to evaluate the advantages and disadvantages of committing to fresh locations.

  1. Is it sufficient to check the local and remote revision numbers to decide if CodingSpectator needs to commit to a fresh location?
  2. Is it sufficient for CodingSpectator to check the revision number of user's directory or it has to check the revision numbers of all the files?
  3. How can CodingSpectator commit data to a fresh location? Should it use svn switch? Can CodingSpectator just create a fresh directory inside the working copy and add it to the repository?
  4. How would committing data to fresh locations affect the existing workspaces? How can we ensure the backwards compatibility of this approach?
  5. Will CodingSpectator need to update its local working copy in spite of committing to fresh locations? Will CodingSpectator need to handle tree conflicts (See issue #309)?
  6. Will committing to fresh locations make it unnecessary to buffer the writes to the watched folder?
reprogrammer commented 13 years ago

@Wanderer777: Another alternative would be to use the svn rm to remove the workspace data remotely and remove the metadata of the local SVN working copy. These commands basically trick CodingSpectator to think that the local data is new and was never uploaded.

reprogrammer commented 13 years ago

@Wanderer777: The test edu.illinois.codingspectator.monitor.tests.TestSubmitterWithConflicts.shouldSubmit() throws the following exception:

org.eclipse.core.runtime.CoreException: Could not write file: .../junit-workspace/.metadata/.plugins/edu.illinois.codingspectator.data/.svn/entries.
    at org.eclipse.core.internal.filesystem.Policy.error(Policy.java:55)
    at org.eclipse.core.internal.filesystem.local.LocalFile.openOutputStream(LocalFile.java:391)
    at org.eclipse.core.filesystem.provider.FileStore.copyFile(FileStore.java:221)
    at org.eclipse.core.filesystem.provider.FileStore.copy(FileStore.java:143)
    at org.eclipse.core.internal.filesystem.local.LocalFile.copy(LocalFile.java:111)
    at org.eclipse.core.filesystem.provider.FileStore.copyDirectory(FileStore.java:181)
    at org.eclipse.core.filesystem.provider.FileStore.copy(FileStore.java:141)
    at org.eclipse.core.internal.filesystem.local.LocalFile.copy(LocalFile.java:111)
    at org.eclipse.core.filesystem.provider.FileStore.copyDirectory(FileStore.java:181)
    at org.eclipse.core.filesystem.provider.FileStore.copy(FileStore.java:141)
    at org.eclipse.core.internal.filesystem.local.LocalFile.copy(LocalFile.java:111)
    at edu.illinois.codingspectator.efs.EFSFile.copyTo(EFSFile.java:67)
    at edu.illinois.codingspectator.monitor.tests.TestSubmitterWithConflicts.makeWatchedFolderOutdated(TestSubmitterWithConflicts.java:54)
    at edu.illinois.codingspectator.monitor.tests.TestSubmitterWithConflicts.modifyLogAndSubmit(TestSubmitterWithConflicts.java:45)
    at edu.illinois.codingspectator.monitor.tests.TestSubmitterWithConflicts.shouldSubmit(TestSubmitterWithConflicts.java:38)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
    at org.junit.runners.BlockJUnit4ClassRunner.runNotIgnored(BlockJUnit4ClassRunner.java:79)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:71)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:49)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:236)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
    at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62)
    at org.eclipse.pde.internal.junit.runtime.PlatformUITestHarness$1.run(PlatformUITestHarness.java:47)
    at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
    at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135)
    at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3563)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3212)
    at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2696)
    at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2660)
    at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2494)
    at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:674)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
    at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:667)
    at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
    at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:123)
    at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.runApp(NonUIThreadTestApplication.java:54)
    at org.eclipse.pde.internal.junit.runtime.UITestApplication.runApp(UITestApplication.java:41)
    at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.start(NonUIThreadTestApplication.java:48)
    at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:344)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:622)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:577)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1410)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1386)
Caused by: java.io.FileNotFoundException: .../junit-workspace/.metadata/.plugins/edu.illinois.codingspectator.data/.svn/entries (Permission denied)
    at java.io.FileOutputStream.open(Native Method)
    at java.io.FileOutputStream.(FileOutputStream.java:194)
    at org.eclipse.core.internal.filesystem.local.LocalFile.openOutputStream(LocalFile.java:382)
    ... 66 more
reprogrammer commented 13 years ago

@Wanderer777: Could you please review my recent changes to the submitter? My next step is to revert my changes that moved some of the CodingSpectator data out of the watched folder.

reprogrammer commented 13 years ago

@Wanderer777: Actually, now that I think more about it, I think it's a good idea to buffer the CodingSpectator data outside the watched folder because it avoids the rare conflicts with data submission. That is, if we buffer the CodingSpectator data out of the watched folder, we no longer risk writing to the watched folder at the same time that Subversion is uploading data from the watched folder to the remote repository. What do you think? Do you agree with me that it's safer to buffer the CodingSpectator data out of the watched folder?

I think CodingTracker doesn't need to buffer its data outside the watched folder because it switches to a temporary file when submission is in progress.

Wanderer777 commented 13 years ago

@reprogrammer: Your changes look good.

As about data buffering, I am not sure why we need a separate solution for CodingSpectator data, i.e. why we can not write it to a temporary file the same way we write CodingTracker data?

To revert back to switching to a temporary file instead of buffering the data, we need to make sure that:

  1. The notification that triggers the switch happens after doAdd but before doCommit (as it was in the previously released version).
  2. We either guarantee that SafeRecorder is notified about the end of a commit operation only when it was notified about its start, or change the implementation of its clients such that they account for the possibility of receiving a notification about the end of a commit operation without a preceding notifications of its start (its a known problem).
  3. Our recent changes to the upload procedure that precede doAdd do not interfere with writing data into the watched directory, since these operations happen before we switch to a temporary file (as far as I can see, they do not interfere).
reprogrammer commented 13 years ago

@Wanderer777: We don't switch CodingSpectator data to a temporary file because it results in intrusively changing the refactoring serialization mechanism of Eclipse.

  1. CodingSpectator needs to get notified before doAdd in order to include its data in the ongoing submission. There are two ways that you could go: First, you could add another hook between doAdd and doCommit to SubmitterListener. Second, you could create the temporary file out of the watched folder and move it to the watched folder during postSubmit if needed. I liked the latter approach because it keeps the API simpler and less dependent on Subversion. Which one do you prefer?
  2. Some listeners might receive the preSubmit notifications and some others may not. I think it'd be complicated for the Submitter to keep track of who has received the preSubmit notifications. Therefore, I suggest that SubmitterListeners keep track of their notifications if they need to.
  3. I didn't understand your third point.
Wanderer777 commented 13 years ago

@reprogrammer:

  1. I prefer to add an additional notification in between doAdd and doCommit because keeping the temporary file in the watched folder ensures that we never lose its content since if something goes wrong, this file will get submitted during the subsequent upload.
  2. I assume this also holds for the preCommit notification that we are going to send in between doAdd and doCommit.
  3. The third point was about ensuring that our changes to the upload mechanism (i.e. the additional checks/operations that might happen before doAdd) would not cause problems if CodingSpectator and CodingTracker keep writing into their log files. The doCommit locks the log files, therefore we had to write into temporary files. The checks and operations that we added do not seem to lock the log files, so I assume it would be safe to keep writing to the log files during these actions.
reprogrammer commented 13 years ago

@Wanderer777:

  1. I think it's safer to avoid the hook between doAdd and doCommit (See item 3).
  2. Yes, in general, it's better to keep the Submitter simple and require the SubmitterListeners to keep track of the notifications that they need.
  3. I think that's a reasonable assumption, but I'm not confident about it. I think this uncertainty is a good reason to avoid the hook between doAdd and doCommit, create the temporary file outside the watched folder and move it to the watched folder during postSubmit.
Wanderer777 commented 13 years ago

@reprogrammer: According to our discussion, I am going to implement a hook between doAdd and doCommit.

reprogrammer commented 13 years ago

@Wanderer777: I just realized that it may not be sufficient to compare the SVN local and remote revision numbers to detect conflicts. Let's say the local revision number of an SVN working copy is 494 and the remote revision number is 495. Assume that the copy of log.txt in the working copy is in conflict with the one in the remote repository. If you do an SVN update operation, Subversion will notify you about the conflict and if you choose to postpone the conflict resolution, the SVN status operation will produce the following output:

?       log.txt.r494
?       log.txt.r495
?       log.txt.mine
C       log.txt

However, the SVN info operation will report that the local revision number has increased to 495 as a result of the SVN update operation. Therefore, CodingSpectator does not detect the existing conflict and fails to submit the data by throwing the following exception:

!ENTRY edu.illinois.codingspectator.monitor.ui 4 0 2011-10-09 23:25:45.587
!MESSAGE Failed to upload CodingSpectator data.
!STACK 0
edu.illinois.codingspectator.monitor.ui.submission.Submitter$SubmissionException: org.tmatesoft.svn.core.SVNException: svn: Commit failed (details follow):
svn: Aborting commit: '.../.metadata/.plugins/edu.illinois.codingspectator.data/log.txt' remains in conflict
    at edu.illinois.codingspectator.monitor.ui.submission.Submitter.submit(Submitter.java:104)
    at edu.illinois.codingspectator.monitor.ui.Uploader$1.run(Uploader.java:56)
    at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Caused by: org.tmatesoft.svn.core.SVNException: svn: Commit failed (details follow):
svn: Aborting commit: '.../.metadata/.plugins/edu.illinois.codingspectator.data/log.txt' remains in conflict
    at org.tmatesoft.svn.core.internal.wc.SVNErrorManager.error(SVNErrorManager.java:85)
    at org.tmatesoft.svn.core.internal.wc.SVNErrorManager.error(SVNErrorManager.java:69)
    at org.tmatesoft.svn.core.wc.SVNCommitClient.doCollectCommitItems(SVNCommitClient.java:1236)
    at org.tmatesoft.svn.core.wc.SVNCommitClient.doCommit(SVNCommitClient.java:825)
    at edu.illinois.codingspectator.monitor.core.submission.RemoteSVNManager.doCommit(RemoteSVNManager.java:65)
    at edu.illinois.codingspectator.monitor.core.submission.SVNManager.doCommit(SVNManager.java:42)
    at edu.illinois.codingspectator.monitor.ui.submission.Submitter.submit(Submitter.java:100)
    ... 2 more
Caused by: org.tmatesoft.svn.core.SVNException: svn: Aborting commit: '.../.metadata/.plugins/edu.illinois.codingspectator.data/log.txt' remains in conflict
    at org.tmatesoft.svn.core.internal.wc.SVNErrorManager.error(SVNErrorManager.java:64)
    at org.tmatesoft.svn.core.internal.wc.SVNErrorManager.error(SVNErrorManager.java:51)
    at org.tmatesoft.svn.core.internal.wc.SVNCommitUtil.harvestCommitables(SVNCommitUtil.java:749)
    at org.tmatesoft.svn.core.internal.wc.SVNCommitUtil.harvestCommitables(SVNCommitUtil.java:983)
    at org.tmatesoft.svn.core.internal.wc.SVNCommitUtil.harvestCommitables(SVNCommitUtil.java:534)
    at org.tmatesoft.svn.core.wc.SVNCommitClient.doCollectCommitItems(SVNCommitClient.java:1208)
    ... 6 more