Community
Participate
Working Groups
Build Identifier: 3.0.2 Is there a reason why ISystemHostPool.cloneHost() doesn't copy the data in the IPropertySetContainer of the host? Reproducible: Always
Created attachment 157479 [details] Proposed fix.
Dave can you please look at this contribution. Xuan may also be interested since she talked about import/export of property sets related to user actions.
Dave this is your area. Please consider.
Property sets are an area that I'd rather not touch any more for the 3.2 release... Tom, do you have a concrete usage scenario where you need this and when? Or did you just notice it during occasional code review?
I do have a concrete use case. We are using RSE to define custom configurations for connecting to embedded systems. These custom configurations contain many settings stored through the PropertySet mechanism. However, these settings are not being copied when the user requests an RSE host copy, e.g., through the Remote System View.
Ok, I see the point. Copying without Properties doesn't really make a lot of sense after all. I have a couple of questions though: 1. It's been a long while like this, do you have significant value if this goes into 3.2 or can you live with 3.2.1 ? 2. If we make the fix, can you commit to testing / verifying the fix? 3. Most property sets that I know of are actually stored in the subsystems and not in the host. Since the issue was not reported earlier, I assume that these are copied as expected -- should be obvious for an FTP connection for instance, where the "passiveMode" property needs to be copied. Likely also for dstore where the ServerLauncher needs to be copied. Did you look at related code from the subsystems and compare what's done there? And, why is your stuff not associated with a Subsystem but with the host? 4. This line is suspicious since you aren't copying the IPropertyType, is this a bug or can you explain: copySet.addProperty(fromProperty.getKey(), fromProperty.getValue()); 5. PropertySets can also have children, your code seems to not copy them (getPersistableChildren()). I'm not sure whether an IHost can actually have them, but at any rate it's something to look out for. Thanks!
And, 6. PropertySet extends PropertySetContainer so you'll also have to copy the result of getPropertySets(). In the longer run, having client code walk down the property set hierarchy just to make a field-by-field copy doesn't look nice. In fact, I'd expect PropertySetContainer to implement Cloneable such that you'd just clone that whole thing ... I can't see why client code should have to walk into the recursive property set structure. Then you'd simply do Host newHost = oldHost.clone(); newHost.setAlias(newAlias); and you're done. This would be an API addition of course so we could only consider in 3.3... Xuan, I'm sure this would help in useractions as well? Dave M, Dave D, has this ever been considered?
(In reply to comment #6 and comment #7) > 1. It's been a long while like this, do you have significant value if this > goes into 3.2 or can you live with 3.2.1 ? We can live without it for now. We have a copy of RSE that we build and distribute with our product, and we have no immediate plans to update. > 2. If we make the fix, can you commit to testing / verifying the fix? Yes. > 3. Most property sets that I know of are actually stored in the subsystems > and not in the host. Since the issue was not reported earlier, I assume > that these are copied as expected -- should be obvious for an FTP > connection for instance, where the "passiveMode" property needs to be > copied. Likely also for dstore where the ServerLauncher needs to be copied. > Did you look at related code from the subsystems and compare what's done > there? And, why is your stuff not associated with a Subsystem but with > the host? Funny thing is we actually have no subsystems. Our main need was a mechanism for gathering embedded connection configuration so that information could be easily shared among multiple launch configurations, especially in the case of multicore. For now we are only using the configuration aspect of RSE. As a result, I'm actually not familiar with the subsystem code, so I didn't look at it in relation to this bug. > 4. This line is suspicious since you aren't copying the IPropertyType, > is this a bug or can you explain: > copySet.addProperty(fromProperty.getKey(), fromProperty.getValue()); Good catch, that's a bug. I guess I managed to overlook this aspect of the design, and have been using only strings. Is this the solution? copySet.addProperty(fromProperty.getKey(), fromProperty.getValue(), fromProperty.getType()); > 5. PropertySets can also have children, your code seems to not copy them > (getPersistableChildren()). I'm not sure whether an IHost can actually > have them, but at any rate it's something to look out for. > 6. PropertySet extends PropertySetContainer so you'll also have to copy > the result of getPropertySets(). I’ll add another patch. (Actually, it will be a code snippet. Hopefully that is sufficient? Otherwise it’s a bit of a hassle to get set up to generate the patch…) Does recursing PropertySets cover point 5 as well?
Created attachment 170208 [details] Replaces previous proposed fix.
Moving some bug milestones from 3.2.1 to 3.2.2.
Bulk moving 3.2.x bugs to 3.3
Created attachment 186198 [details] patch to copy property set during host clone
I just noticed this one - not sure how I missed it earlier. Anyway, I've converted the proposed fix into patch format. Tom, could you provide a legal disclaimer?
I, Tom Hochstein, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer to make this contribution under the EPL.
Thanks for the quick response, Tom. I'll commit this to the HEAD stream. Does this change need to be backported to an earlier release?
Thanks, Dave, but we don't require any backporting on this.
(In reply to comment #16) > Thanks, Dave, but we don't require any backporting on this. Okay great. I've committed the changes. Sorry about the long wait.
Comment on attachment 170208 [details] Replaces previous proposed fix. We need the iplog+ on the patch uploaded by Tom, in order to get proper IP log.
Looking at the patch, I think it's incorrect in the case of nested property sets. The code uses fromSet.getPropertySets() in order to list the sets nested in fromSet; but it clones them all flat into the connection ("copy") instead of into the nested copySet. Reopening for comment.
Created attachment 186407 [details] Fix nested property sets. Thanks, Martin, that's a good catch. The attached patch should fix it, and includes a JUnit test. I also added a line to the copyright for your other comment. Does that solve the IPLOG issue?
Comment on attachment 186407 [details] Fix nested property sets. the new patch looks good to me
Thanks for the junits as well. I've committed the update to cvs.
Looks like the test failed on tonight's N-build. FYI, there is a properties file to define test connections for unittests. https://hudson.eclipse.org/hudson/job/cbi-tm-3.2-nightly/191/ Error Message Failed to find source connection TestHost1 Stacktrace junit.framework.AssertionFailedError: Failed to find source connection TestHost1 at org.eclipse.rse.tests.core.connection.RSEConnectionTestCase.testConnectionCopy(RSEConnectionTestCase.java:122) at org.eclipse.rse.tests.core.RSECoreTestCase.runTest(RSECoreTestCase.java:425)
The test I added, org.eclipse.rse.tests.core.connection.RSEConnectionTestCase.testConnectionCopy(), requires testConnectionCreation() to run before it, the same requirement that testConnectionRemoval() makes. However, as far as I can tell, the nightly build tests are run in alphabetical order, and so testConnectionCopy fails. A simple fix would be to just merge testConnectionCopy into testConnectionCreation. Would that be sufficient? (I would provide a patch, but I seem to be having problems with my firewall.)
Tom, I think that no Unittest should ever make assumptions about what has happened before (or what will happen afterwards). A Unittest should assume a pristine environment and clean up after it has run - otherwise it's not a unittest. I'd also like to keep individual tests separate, testing one specific aspect each only: testConnectionCreation should test creation ONLY. Looks like what this is what we want: 1.) tearDown() should delete any connections that any test has created 2.) Refactor any tests that require an already-created connection to either call a common method for creating that connection, or to leverage the setUp() method to create the connection for them.
(In reply to comment #25) > Tom, I think that no Unittest should ever make assumptions about what has > happened before (or what will happen afterwards). A Unittest should assume a > pristine environment and clean up after it has run - otherwise it's not a > unittest. > I'd also like to keep individual tests separate, testing one specific aspect > each only: testConnectionCreation should test creation ONLY. > Looks like what this is what we want: > 1.) tearDown() should delete any connections that any test has created > 2.) Refactor any tests that require an already-created connection to > either call a common method for creating that connection, or to > leverage the setUp() method to create the connection for them. Sounds good. I should be able to work on this today if you'd like me to.
yes please :)
Created attachment 186687 [details] Fix test for nightly build. I moved the test to a dedicated test suite, HostCopyTest.java, using the existing test suite HostMoveTest.java as a model.
Dave can you please work on getting Tom's latest patch in? Last night's nightly failed again with the known failure. https://hudson.eclipse.org/hudson/job/cbi-tm-3.2-nightly/192/testReport/org.eclipse.rse.tests.core.connection/RSEConnectionTestCase/testConnectionCopy/
(In reply to comment #29) > Dave can you please work on getting Tom's latest patch in? > > Last night's nightly failed again with the known failure. > https://hudson.eclipse.org/hudson/job/cbi-tm-3.2-nightly/192/testReport/org.eclipse.rse.tests.core.connection/RSEConnectionTestCase/testConnectionCopy/ I've committed the new patch to cvs.
