[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1331


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

GitHub user syed reopened a pull request:

https://github.com/apache/cloudstack/pull/1331

Fix Sync of template.properties in Swift

When using Swift as a secondary storage, we create a template.properties 
file which stores the metadata about the template. Currently the data which is 
present in the file is incorrect which leads to templates becoming unavailable 
after they are downloaded. This fix makes sure that the template.properties has 
the correct "path" set so that templates are available. 

I've also done a bit of cleanup and made the code bit more clean. 


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/syed/cloudstack swift-restart-fix

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1331.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1331


commit f3b8c7e0f2720639f1db77b15e0791ec70537a29
Author: Syed 
Date:   2015-12-14T22:37:28Z

Fix Sync of template.properties in Swift




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-219166142
  
@DaanHoogland  sure ... doesn't hurt



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-219162095
  
@swill I got the file from jenkins and it points to 
`utils/testsmallfileinactive` ... I have no idea where this file came from


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-219150983
  
@syed failed again.  :(


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-219074192
  
@syed can you force push or close and reopen the PR to kick off travis and 
jenkins?  I think this one seems to be in a pretty good state now...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-219073755
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 0
   Errors: 0
 Duration: 4h 18m 48s
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU/runinfo.txt)

**`/tmp/MarvinLogs/test_network_R3P7FB:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_network_R3P7FB/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_network_R3P7FB/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_network_R3P7FB/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_HKWN9Z:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_vpc_routers_HKWN9Z/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_vpc_routers_HKWN9Z/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_vpc_routers_HKWN9Z/runinfo.txt)


Uploads will be available until `2016-07-13 02:00:00 +0200 CEST`

*Comment created by [`upr comment`](https://github.com/cloudops/upr).*



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-218591057
  
@syed can you have a look at this one.

I am not sure why, but I am having a lot of trouble with this PR.  I have 
not been able to get a test run to actually works.  I have tried to run CI on 
this 3 times and I always get the following:

```
INFO  [c.c.r.ResourceManagerImpl] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) 
(logid:2044d020) Trying to add a new host at http://kvm1 in data center 1
INFO  [o.a.c.f.j.i.AsyncJobManagerImpl] 
(AsyncJobMgr-Heartbeat-1:ctx-412302f9) (logid:5b139200) Begin cleanup expired 
async-jobs
INFO  [o.a.c.f.j.i.AsyncJobManagerImpl] 
(AsyncJobMgr-Heartbeat-1:ctx-412302f9) (logid:5b139200) End cleanup expired 
async-jobs
INFO  [o.a.c.e.o.NetworkOrchestrator] (Network-Scavenger-1:ctx-a52a3147) 
(logid:86cda124) NetworkGarbageCollector uses '10' seconds for GC interval.
INFO  [c.c.h.k.d.LibvirtServerDiscoverer] (ApiServer-1:ctx-7bf8070e 
ctx-cdcb3f39) (logid:2044d020) cloudstack agent setup command failed: 
cloudstack-setup-agent  -m 192.168.22.61 -z 1 -p 1 -c 1 -g 
57acd541-cd37-34ea-af6a-dc6ecd007325 -a --pubNic=cloudbr0 --prvNic=cloudbr1 
--guestNic=cloudbr0 --hypervisor=kvm
WARN  [c.c.r.ResourceManagerImpl] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) 
(logid:2044d020) Unable to find the server resources at http://kvm1
INFO  [c.c.u.e.CSExceptionErrorCode] (ApiServer-1:ctx-7bf8070e 
ctx-cdcb3f39) (logid:2044d020) Could not find exception: 
com.cloud.exception.DiscoveryException in error code list for exceptions
WARN  [o.a.c.a.c.a.h.AddHostCmd] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) 
(logid:2044d020) Exception: 
com.cloud.exception.DiscoveryException: Unable to add the host
at 
com.cloud.resource.ResourceManagerImpl.discoverHostsFull(ResourceManagerImpl.java:802)
at 
com.cloud.resource.ResourceManagerImpl.discoverHosts(ResourceManagerImpl.java:597)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at 
org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:317)
at 
org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183)
at 
org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150)
at 
org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:91)
at 
org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
at 
org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:204)
at com.sun.proxy.$Proxy159.discoverHosts(Unknown Source)
at 
org.apache.cloudstack.api.command.admin.host.AddHostCmd.execute(AddHostCmd.java:142)
at com.cloud.api.ApiDispatcher.dispatch(ApiDispatcher.java:150)
at com.cloud.api.ApiServer.queueCommand(ApiServer.java:698)
at com.cloud.api.ApiServer.handleRequest(ApiServer.java:529)
at com.cloud.api.ApiServer.handle(ApiServer.java:434)
at 
org.apache.http.protocol.HttpService.doService(HttpService.java:423)
at 
org.apache.http.protocol.HttpService.handleRequest(HttpService.java:341)
at 
com.cloud.api.ApiServer$WorkerTask.runInContext(ApiServer.java:1236)
at 
org.apache.cloudstack.managed.context.ManagedContextRunnable$1.run(ManagedContextRunnable.java:49)
at 
org.apache.cloudstack.managed.context.impl.DefaultManagedContext$1.call(DefaultManagedContext.java:56)
at 
org.apache.cloudstack.managed.context.impl.DefaultManagedContext.callWithContext(DefaultManagedContext.java:103)
at 
org.apache.cloudstack.managed.context.impl.DefaultManagedContext.runWithContext(DefaultManagedContext.java:53)
at 
org.apache.cloudstack.managed.context.ManagedContextRunnable.run(ManagedContextRunnable.java:46)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)
INFO  [c.c.a.ApiServer] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) 
(logid:2044d020) Unable to add the host
```

The fact that both Jenkins and Travis are failing may also be relevant.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or 

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-218154430
  
@swill the problem is because of `LocalNfsSecondaryStorageResourceTest`. 
Before my commit, tests were ignored for this module probably because of the 
above file. Since I added a new file and enabled tests, the file started 
bothering again. For now I've silenced it. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-218056377
  
@syed this is not compiling for me.  Can you review?

```
[INFO] 

[INFO] Building Apache CloudStack Plugin - RabbitMQ Event Bus 4.7.2-SNAPSHOT
[INFO] 

[WARNING] *
[WARNING] * Your build is requesting parallel execution, but project  *
[WARNING] * contains the following plugin(s) that are not marked as   *
[WARNING] * @threadSafe to support parallel building. *
[WARNING] * While this /may/ work fine, please look for plugin updates*
[WARNING] * and/or request plugins be made thread-safe.   *
[WARNING] * If reporting an issue, report it against the plugin in*
[WARNING] * question, not against maven-core  *
[WARNING] *
[WARNING] The following plugins are not marked @threadSafe in Apache 
CloudStack Plugin - RabbitMQ Event Bus:
[WARNING] org.apache.maven.plugins:maven-site-plugin:3.1
[WARNING] *
[INFO] 
[INFO] --- maven-checkstyle-plugin:2.11:check (cloudstack-checkstyle) @ 
cloud-mom-rabbitmq ---
[INFO] Starting audit...
Audit done.

[INFO] 
[INFO] --- maven-remote-resources-plugin:1.3:process (default) @ 
cloud-mom-rabbitmq ---
log4j:ERROR Could not parse url 
[file:/data/git/cs1/cloudstack/services/secondary-storage/server/test/../conf/log4j.xml].
java.io.FileNotFoundException: 
/data/git/cs1/cloudstack/services/secondary-storage/server/test/../conf/log4j.xml
 (No such file or directory)
at java.io.FileInputStream.open(Native Method)
at java.io.FileInputStream.(FileInputStream.java:146)
at java.io.FileInputStream.(FileInputStream.java:101)
at 
sun.net.www.protocol.file.FileURLConnection.connect(FileURLConnection.java:90)
at 
sun.net.www.protocol.file.FileURLConnection.getInputStream(FileURLConnection.java:188)
at 
org.apache.log4j.xml.DOMConfigurator$2.parse(DOMConfigurator.java:765)
at 
org.apache.log4j.xml.DOMConfigurator.doConfigure(DOMConfigurator.java:871)
at 
org.apache.log4j.xml.DOMConfigurator.doConfigure(DOMConfigurator.java:778)
at 
org.apache.log4j.helpers.OptionConverter.selectAndConfigure(OptionConverter.java:526)
at org.apache.log4j.LogManager.(LogManager.java:127)
at org.apache.log4j.Logger.getLogger(Logger.java:117)
at 
com.cloud.resource.ServerResourceBase.(ServerResourceBase.java:45)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:195)
at javassist.runtime.Desc.getClassObject(Desc.java:43)
at javassist.runtime.Desc.getClassType(Desc.java:152)
at javassist.runtime.Desc.getType(Desc.java:122)
at javassist.runtime.Desc.getType(Desc.java:78)
at 
org.apache.cloudstack.storage.resource.NfsSecondaryStorageResourceTest.setUp(NfsSecondaryStorageResourceTest.java:49)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at 
org.junit.internal.runners.MethodRoadie.runBefores(MethodRoadie.java:132)
at 
org.junit.internal.runners.MethodRoadie.runBeforesThenTestThenAfters(MethodRoadie.java:95)
at 
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.executeTest(PowerMockJUnit44RunnerDelegateImpl.java:294)
at 
org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTestInSuper(PowerMockJUnit47RunnerDelegateImpl.java:127)
at 
org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTest(PowerMockJUnit47RunnerDelegateImpl.java:82)
at 
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runBeforesThenTestThenAfters(PowerMockJUnit44RunnerDelegateImpl.java:282)
at 
org.junit.internal.runners.MethodRoadie.runTest(MethodRoadie.java:86)
at org.junit.internal.runners.MethodRoadie.run(MethodRoadie.java:49)
at 

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-217971133
  
@rafaelweingartner @jburwell Sorry for the late reply to this. I was on 
vacation and returned today. I've decided to use John's approach for the 
logging test as I feel it is more natural. Also the dicussion about `final 
static` being immutable and skipped by GC makes me feel that using an appender 
makes more sense. Furthermore, the `TestAppender` class provided by @jburwell 
is generic so that other tests can use it too. 

I will sqash the commits as it looking pretty good now unless you guys have 
any more comments. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-215851189
  
@jburwell, that is great that we understood each other ;)

Your questions is a great one, why are we discussion this, and why testing 
that. Actually, I do not care much about the writing of test cases to check if 
something is logged.

The point is that there is that “cleanupStagingNfs” method. If we want 
to write a nice test to it, we would have to test the happy day flow; that 
means if the method is creating the “DeleteCommand” object and then if it 
call the “execute” method with that object. That is one test case. I see 
that kind of test as an integration test; meaning, a test if a method is using 
some other method.

Having said that, I see another flow of execution; we might have an 
exception. So, if an exception occurs, what happens? By the code, today we log 
the exception and the execution continues.

Then we would have to ask ourselves, do we want to enforce that? If someone 
silences that exception, would we like to catch that?

If someone re-throws the exception, using a runtime one; would we like to 
detect that?

At first sight, I think that we should guarantee both executions flows. 
But, if it becomes a burden, we can let it go.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-215620806
  
@rafaelweingartner I apologize -- I did not understand that you were 
focused on ``final`` not ``final static``.  You are correct, ``final`` has 
nothing to do with garbage collection -- it is about thread safety.  There are 
very few (and rare) circumstances where a ``static`` reference should be 
mutable because it accessible across thread boundarys.  When they need to be 
mutable, special care must be taken to ensure exclusive access (e.g. 
``synchronized`` blocks, ``ReentrantLock``, ``AtomicReference``, etc).  
Therefore, I always put the two together.  In this case, the logger is a 
constant value and should be declared ``final``.  Sacrificing thread-safety for 
unit test is not an appropriate trade-off in my opinion.

Stepping back a bit, why are we investing so much time and effort to test a 
``DEBUG`` log message?  ``DEBUG`` level messages should only be for developers 
during development of the system.  Not only is a low value item to test, it 
makes the test case brittle as there should never be any guarantees about log 
messages at ```DEBUG`` and ``TRACE`` log levels.  In my opinion, @syed should 
remove this part of the test entirely.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-214529726
  
I do not find them (final static) purely cosmetics (we are using too much 
this expression lately).  I understand its use, as I presented the references 
about the GC and static variables. Maybe we could use only the static keyword? 
That is what I am saying. The final keyword would work more like an assurance, 
so no one can change that reference during runtime. But, I believe we do not 
need such guarantees, right?

I could not find anything (specs or guides from Oracle or OpenJDK) saying 
that the "final" keyword would provide such improvements. Maybe you could help 
me find something to clarify my doubts?

About logging from static methods, I do not find that argument appealing. 
Even though static methods may be appealing, they are easier to use and code, 
not needing to integrate into a framework life cycle to wire all of the 
dependencies. I believe that if we have an “util” class with static 
methods, then it (the util class) will have a static variable “Logger”; 
now, mixing static methods into a singleton;  I see that as an anti-pattern. 
This is very personal, I like to delegate very specific tasks to each class. I 
believe that facilitates the design of the software and the writing of test 
cases (both unit and integration one)

I understand your feelings about Mockc. I had the same some time ago when I 
started working with TDD. At that time I used Easymock. But still, the point is 
that, once we start writing test cases (the unit ones), we get into a moment in 
which we have to write integration tests. That means a method that uses few 
methods that are self-contained and unit tested. Therefore, we do not need to 
test the whole method (we already assured that the units are working), but only 
check if the method is calling the methods (units) it should (checking the 
order), with the parameters that are expected and at the end returning what we 
expect it to return. To facilitate that job we would need to use Mocks, then we 
would not have to prepare complicated set ups to write our tests.

About the overhead problems to the GC, as I said, I am talking about 
singletons, they are created only once in the whole application life cycle. 
Therefore, they would not cause overhead problems with the GC.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-214518411
  
@rafaelweingartner yes, as I have said, ``final statics`` are not evaluated 
for GC.  A single case is not a noticeable problem.  However, if made all 
loggers were made instance variables, given the number we have, it would become 
a problem.  You stated that your preference was to make loggers instance 
variables, and that they being ``final static`` was purely cosmetic.  My point 
is that there are both performance and capability motivations to make them 
``final static`` (i.e. the ability to log from static methods and accumulated 
GC overhead).

Personally, I use Mockito (and its ilk) sparingly.  Namely, I only use it 
in circumstances where language constructs are unable to mock a mechanism.  
First, I find it more difficult to understand the intentions of a test using 
Mockito rather than pure Java as it layers another set of abstractions onto the 
test.  Second, Mockito approaches discourage reuse.  We could use Mockito to 
mock logging tests in this case.  However, we have done nothing to make testing 
of expected logging easier for future test cases.

It's most important to me that we do not start turning references to 
constant values into instance variables due to potential GC churn.  It's less a 
memory size issue than it is about the length of GC operations and CPU 
overhead.  The Mockito vs. ``TestAppender`` approach is up to @syed and what he 
feels most comfortable implementing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-214509189
  
@jburwell, @syed, sorry the long post, I did a research on a few thing and 
I would like to share with you guys.

I was just looking at the state of this PR, and I noticed that the number 
of lines added jumped to 300+; most of those lines are needed in order to write 
a test case using the “TestAppender” approach. 
Please do not take me in a bad way; I find discussions like this very 
healthy for the future of the project.  I also know that the class 
“TestAppender” is there to be reused in some other tests that want to check 
the use of log; but still, it feels pretty complicated to me. Giving that, do 
you see why when I created the test as an example for @Syed, I used the Mocking 
approach? It feels simpler and more natural to write tests using that approach, 
at least to me.

Additionally, I was curious when you mentioned that the use of “static 
final” delimiters could optimize the GC, given that it (the GC) would not 
check if those attributes have or not to be garbage collected. I had never 
heard that before, so I tried to find something online. If you have some 
reliable reference about that, could you share?

I tried to find some specs or guideline documents from either Oracle or 
OpenJDK without much success.  However, I found something like you said to the 
android JVM [1], but that would not be our case. Then, I read some articles 
(not scientific ones) from IBM [2] and Oracle [3] about the tuning of java 
code. But still, they did not mention anything related to what you said. Then, 
I decided to revisit a forum that I did not visit for a long time, since the 
time of my first Java certification (I am definitely getting old :(), the 
coderanch [4]. There I found something that may be related to what you said 
[5]. There was a discussion there about the GC and static variables; at some 
point, someone highlights that “Static variables are destroyed when the class 
is unloaded”.  After that, I also found this [6] on Stack overflow, in which 
it is described that “Static variables cannot be elected for garbage 
collection while the class is loaded. They can be collected when the respective
  class loader (that was responsible for loading this class) is itself 
collected for garbage.”. I believe that was the point you wanted to make, 
right? Static variables are not GCed by the GC, they are not even checked.
If that was the case, it would have nothing to do with the “final” word 
per se, but rather with the use of the “static” word.

After having said that, perhaps we could benefit from both words? I mean we 
could still use the “Logger” variable as static, but not final; then, we 
would be able to write a test case using Mockito (as the first example I 
presented to @Syed), which would add less code.
What do you think about that?

Additionally, I had taken a look at some spring framework code. Their 
framework is not only the base of ACS but also many other huge projects; so, I 
thought it would be interesting to see how they use the logger variables. They 
use their “logger” attributes as Object variables and not Classes. With 
that approach when you extend their code, you “get for free” a logger 
instance to be used. I believe that is why I am so used to loggers being object 
attributes. I might have been working too much with their components and 
frameworks.

When we do that, we can avoid the following example that happens in ACS:
Let’s take the example of “NfsSecondaryStorageResource” class. That 
class is an intermediate class to singletons (LocalNfsSecondaryStorageResource, 
MockLocalNfsSecondaryStorageResource, PremiumSecondaryStorageResource and 
SimulatorSecondaryStorageResource). All of them also have a Logger instance for 
their respective class. Also, the “NfsSecondaryStorageResource” extends the 
“ServerResourceBase” that has a Logger instance too. In total, we have 5 
Logger instances. One for each class, giving that all of them are static 
attributes. If we used an approach similar to the one that is used by Spring-*, 
we would have one “Logger” instance for each singleton, which would 
represent 4 logger instances.

I did some tests, and the approximated size of a Logger instance is ~820 
bytes. So, if we save the instantiation of a few of this we can reduce a little 
bit of the use of ACS memory, giving that due to the way we create “Logger” 
objects today, we have an instance even for classes that are not object per se, 
but intermediated classes in a hierarchy of singletons.

[1] http://developer.android.com/training/articles/perf-tips.html
[2] http://www.ibm.com/developerworks/library/j-jtp01274/
[3] 
https://docs.oracle.com/cd/E26576_01/doc.312/e24936/tuning-apps.htm#GSPTG00161

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r60742612
  
--- Diff: 
services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java
 ---
@@ -18,91 +18,68 @@
  */
 package org.apache.cloudstack.storage.resource;
 
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileNotFoundException;
-import java.io.IOException;
-import java.net.URI;
-import java.util.Map;
-import java.util.Properties;
-
-import javax.naming.ConfigurationException;
-
-import junit.framework.Assert;
-import junit.framework.TestCase;
-
+import org.apache.cloudstack.storage.command.DeleteCommand;
+import org.apache.cloudstack.storage.to.TemplateObjectTO;
 import org.apache.log4j.Level;
 import org.apache.log4j.Logger;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mockito;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
-import com.cloud.utils.PropertiesUtil;
-import com.cloud.utils.exception.CloudRuntimeException;
+import java.io.BufferedWriter;
+import java.io.FileWriter;
+import java.io.StringWriter;
 
-public class NfsSecondaryStorageResourceTest extends TestCase {
-private static Map testParams;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
 
-private static final Logger s_logger = 
Logger.getLogger(NfsSecondaryStorageResourceTest.class.getName());
+@RunWith(PowerMockRunner.class)
+public class NfsSecondaryStorageResourceTest {
 
-NfsSecondaryStorageResource resource;
+private NfsSecondaryStorageResource resource;
 
 @Before
-@Override
-public void setUp() throws ConfigurationException {
-s_logger.setLevel(Level.ALL);
+public void setUp() {
 resource = new NfsSecondaryStorageResource();
-resource.setInSystemVM(true);
-testParams = PropertiesUtil.toMap(loadProperties());
-resource.configureStorageLayerClass(testParams);
-Object testLocalRoot = testParams.get("testLocalRoot");
-if (testLocalRoot != null) {
-resource.setParentPath((String)testLocalRoot);
-}
 }
 
 @Test
-public void testMount() throws Exception {
-String sampleUriStr = 
"cifs://192.168.1.128/CSHV3?user=administrator=1pass%40word1=bar";
-URI sampleUri = new URI(sampleUriStr);
-
-s_logger.info("Check HostIp parsing");
-String hostIpStr = resource.getUriHostIp(sampleUri);
-Assert.assertEquals("Expected host IP " + sampleUri.getHost() + " 
and actual host IP " + hostIpStr + " differ.", sampleUri.getHost(), hostIpStr);
-
-s_logger.info("Check option parsing");
-String expected = 
"user=administrator,password=1pass@word1,foo=bar,";
-String actualOpts = resource.parseCifsMountOptions(sampleUri);
-Assert.assertEquals("Options should be " + expected + " and not " 
+ actualOpts, expected, actualOpts);
-
-// attempt a configured mount
-final Map params = 
PropertiesUtil.toMap(loadProperties());
-String sampleMount = (String)params.get("testCifsMount");
-if (!sampleMount.isEmpty()) {
-s_logger.info("functional test, mount " + sampleMount);
-URI realMntUri = new URI(sampleMount);
-String mntSubDir = resource.mountUri(realMntUri);
-s_logger.info("functional test, umount " + mntSubDir);
-resource.umount(resource.getMountingRoot() + mntSubDir, 
realMntUri);
-} else {
-s_logger.info("no entry for testCifsMount in " + 
"./conf/agent.properties - skip functional test");
-}
-}
+@PrepareForTest(NfsSecondaryStorageResource.class)
+public void testSwiftWriteMetadataFile() throws Exception {
+String expected = 
"uniquename=test\nfilename=testfile\nsize=100\nvirtualsize=1000";
 
-public static Properties loadProperties() throws 
ConfigurationException {
-Properties properties = new Properties();
-final File file = 
PropertiesUtil.findConfigFile("agent.properties");
-if (file == null) {
-throw new ConfigurationException("Unable to find 
agent.properties.");
-}
-s_logger.info("agent.properties found at " + 
file.getAbsolutePath());
-

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r60742145
  
--- Diff: 
services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/TestAppender.java
 ---
@@ -0,0 +1,173 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+package org.apache.cloudstack.storage.resource;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableMap;
+import org.apache.log4j.AppenderSkeleton;
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.apache.log4j.spi.LoggingEvent;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Pattern;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Strings.isNullOrEmpty;
+import static java.lang.String.format;
+import static org.apache.log4j.Level.ALL;
+import static org.apache.log4j.Level.DEBUG;
+import static org.apache.log4j.Level.ERROR;
+import static org.apache.log4j.Level.FATAL;
+import static org.apache.log4j.Level.INFO;
+import static org.apache.log4j.Level.OFF;
+import static org.junit.Assert.fail;
+
+/**
+*
+* Tracks one or more patterns to determine whether or not they have been
+* logged. It uses a streaming approach to determine whether or not a 
message
+* has a occurred to prevent unnecessary memory consumption. Instances of 
this
+* of this class are created using the {@link TestAppenderBuilder}.
+*
+* To use this class, register a one or more expected patterns by level as 
part
+* of the test setup and retain an reference to the appender instance. 
After the
+* expected logging events have occurred in the test case, call
+* {@link TestAppender#assertMessagesLogged()} which will fail the test if 
any of the
+* expected patterns were not logged.
+*
+*/
+public final class TestAppender extends AppenderSkeleton {
+private final static String APPENDER_NAME = "test_appender";
+private final ImmutableMap 
expectedPatternResults;
+private TestAppender(final Map 
expectedPatterns) {
+super();
+expectedPatternResults = ImmutableMap.copyOf(expectedPatterns);
+}
+protected void append(LoggingEvent loggingEvent) {
+checkArgument(loggingEvent != null, "append requires a non-null 
loggingEvent");
+final Level level = loggingEvent.getLevel();
+checkState(expectedPatternResults.containsKey(level), "level " + 
level + " not supported by append");
+for (final PatternResult patternResult : 
expectedPatternResults.get(level)) {
+if 
(patternResult.getPattern().matcher(loggingEvent.getRenderedMessage()).matches())
 {
+patternResult.markFound();
+}
+}
+}
+public void close() {
+// Do nothing ...
+}
+public boolean requiresLayout() {
+return false;
+}
+public void assertMessagesLogged() {
+final List unloggedPatterns = new ArrayList<>();
+for (final Map.Entry 
expectedPatternResult : expectedPatternResults.entrySet()) {
+for (final PatternResult patternResults : 
expectedPatternResult.getValue()) {
+if (!patternResults.isFound()) {
+unloggedPatterns.add(format("%1$s was not logged for 
level %2$s",
+patternResults.getPattern().toString(), 
expectedPatternResult.getKey()));
+}
+}
+}
+if (!unloggedPatterns.isEmpty()) {
+fail(Joiner.on(",").join(unloggedPatterns));
+}
+}
+private static final class 

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r60740846
  
--- Diff: 
services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/TestAppender.java
 ---
@@ -0,0 +1,173 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+package org.apache.cloudstack.storage.resource;
--- End diff --

Since there is nothing about that is specific to secondary storage, Why not 
place this class in a more general package such as ``com.cloud.test.utils``?  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-212907833
  
@jburwell @rafaelweingartner I've added the test for checking the logged 
message using the gist provided by @jburwell. I've also added a `log4j.xml` 
because the test was not running without one. The test now fails to assert the 
message in `assertMessagesLogged`. @jburwell am I using this correctly? 




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-212599647
  
@rafaelweingartner hopefully, the appender I specified in the 
[gist](https://gist.github.com/jburwell/98b6d94c57f409b5b778ffee6a8a12b1) 
should be a good start (if not nearly complete) common appender that can be 
used across all test cases.  Since I don't have a test case to exercise, 
handing it off to @syed to complete, if he desires, seems like the most 
pragmatic way to complete it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-212598453
  
@jburwell I agree with you. The key point here is 
> instances that are rapidly allocated and deallocated
Singletons are not meant to be deallocated all the time. 

I just created that commit to be used as an example for @syed; I have just 
done the same thing for the other method he created, giving that we are in very 
different time zones, and if we try to develop a more fine discussion and 
explanation here, I thought it could use too much of hist time. So, some 
examples for him would be easier to understand and grasp the idea of a test 
case.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-212596332
  
@rafaelweingartner the garbage collection issue is not the 
allocation/deallocation, but the reachability calculation that the garbage 
collector must perform for every instance member (i.e. determining whether or 
not that instance needs to be collected).  Since it, ultimately, the logger is 
a singleton, this reachability check is completely unnecessary.  In contrast, 
the garbage collector skips this check for ``static final`` members as it is 
guaranteed to be reachable for the lifetime of the JVM.  While the overhead of 
this check is relatively small, making logger (or any other constant values) 
instance members on classes whose instances  are rapidly allocated and 
deallocated accrues quickly.

@syed Done properly, the appender approach can be made reusable for other 
test cases to verify logging behavior.  I built a [custom 
appender](https://gist.github.com/jburwell/98b6d94c57f409b5b778ffee6a8a12b1) 
that allows monitoring of a logger for one or more patterns and assert that 
logging occurred as expected.   The following is an example of how to use it in 
this test case:

```
public class NfsSecondaryStorageResourceTest {

private TestAppender testAppender;

   @Before
   public void setup() {
   testAppender = new 
TestAppenderBuilder().addExpectedPattern(Level.DEBUG, "Failed to clean up 
staging area:\*+").build();
   Logger logger = Logger.getLogger(NfsSecondaryStorageResource.class);
   TestAppender.safeAddAppender(logger, testAppender);
   }

   @Test
   public void testSwiftWriteMetadataFile() throws Exception {
   // test operations and asserts
   testAppender.assertMessagesLogged();
   }
```

The class likely needs a little work, and definitely, some testing.  I put 
it together quickly to demonstrate  a reusable approach to testing logging.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-212529350
  
@syed, sorry the delay, I haven't had much time.
I noticed that this class is being ignored. The tests are not properly 
coded (test should not rely on external files). There is a PR 
(https://github.com/apache/cloudstack/pull/1499) from a colleague that is 
addressing exactly that.

On the meantime, this is a pretty tricky method to test; but, there is a 
nice way to do so.

I opened a PR for your branch with an example, so you can see how to 
address the test of that method you extracted.
Also, about the other test "testCleanupNfsStaging", I suggest you 
experiment the other approach pointed out by @jburwell, so you can use both and 
then it would be easier for you to create an opinion. The way I proposed in my 
PR, was to be used as an illustration for you.
Any other doubts/problems, you can just ping me.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-211995051
  
@rafaelweingartner I've added the test mocking the `BufferedWriter`. When I 
tried to run the tests I found that the test required `agent.properties` and 
`log4j.xml` which I am not sure where they are. Also something that I found was 
that tests for this module (`cloud-secondary-storage`) are disabled [see 
here](https://github.com/apache/cloudstack/blob/master/services/secondary-storage/server/pom.xml#L30)
 and from the history it appears that this `SkipTests` was present from the 
beigninging. 

Any clues on fixing it? 



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-211485538
  
I agree with you @syed, it is always good to discuss the patterns we use.
That example you brought up also works; it would do the same tricky to 
test, but using an appender stead of mocking. I am good with that too; I just 
coded differently, because I do not like much the idea of getting log entries 
by hand. But, I am willing to go either way. If you go the other way, please 
revert my commit, so the history does not get messed up.

For the new tests you have to write, you could mock the creation of the 
Object buffered writer, to one that you can control and check if the 
“write” method is been used as expected. In other words, writing the 
strings you expect to write.
Here there is an example of that for the File class (it is not the same, 
but it will give you an idea):
http://stackoverflow.com/questions/11849728/simulate-file-in-java


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-211455173
  
Loving the discussion here guys! Based on the comments by @jburwell I found 
http://stackoverflow.com/questions/1827677/how-to-do-a-junit-assert-on-a-message-in-a-logger
  @rafaelweingartner I would like your opinion on this. 

I've also added a function which basically writes the `template.properties` 
file out to the disk. Now I was thinking of mocking the `BufferedWriter` in 
that function and somehow catch the output and assert it with an expected 
output but the writer is a variable which is local to that function. How do 
mock that @rafaelweingartner 




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-210086309
  
I see your point in regards to the garbage collector; but, that class is a 
singleton, it is not going to be allocated and deallocated.

About the point about of using the log in static method; well, that class 
is a singleton and I believe that they should not have static method, we are 
using a dependency injection framework, we should wire things up using the 
lifecycle of the framework we choose and not try to work things out with static 
methods.

Actually I see that the need for unit tests reflects on the code. Code with 
50, 100 and even more lines cannot be properly tested. The TDD approach affects 
a lot on the design and writing of code.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-210081688
  
@rafaelweingartner The issue is not one of style but performance and 
availability.  The garbage collector skips reachability evaluations of ``final 
static`` fields where it must perform those for instance variables.  This cost 
adds up when a large number of object is allocated and deallocated.  Second, 
you cannot log from a static method.  Our standard in the codebase is to have 
``final static`` loggers, and I don't see a compelling reason to step away from 
it.

In terms of the Mockito approach, I wouldn't have an opinion if the unit 
test where not requiring us to change the class.  The test should always be the 
servant of the system not the other way around.  Personally, I find mocks to be 
an obfuscation especially when the thing being mocked provides a standard 
mechanism to observe side-effects.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-210078579
  
@jburwell I agree with you that all static can be final. I disagree that 
loggers should be static, but that is a matter of taste. I understand the cache 
methods of Log4J, but still I do not like much of using static variables. It 
feels more natural to use the instance attribute when logging.

I find it much more complications to add an appender and then use it to 
test if some method has been used or some message logged.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r59762357
  
--- Diff: 
services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java
 ---
@@ -88,6 +93,19 @@ public void testMount() throws Exception {
 }
 }
 
+@Test
+public void testCleanupNfsStaging(){
+TemplateObjectTO templateMock = 
Mockito.mock(TemplateObjectTO.class);
+Exception exception = new Exception();
+resource.logger = Mockito.mock(Logger.class);
+
+Mockito.doNothing().when(resource.logger).debug("Failed to clean 
up staging area:", exception);
+
Mockito.when(resource.execute(any(DeleteCommand.class))).thenThrow(exception);
--- End diff --

I didn't use that other approach because I think it adds more 
complications. I find it easier to use the Mockito and then just checking if 
the method was used. It feels more natural.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-210066343
  
@rafaelweingartner all statics should ``final`` unless there is explicit 
handling for their value changing due to thread safety issues. For loggers, 
they should always be ``private`` and ``static``.  Log4j provides the means to 
retrieve the same logger across threads and contexts by name.  Therefore, if 
you want 5 classes to share the same logger, retrieve the logger for that class 
by name in each class to avoid the potential pitfalls of static shadowing (i.e. 
instead of declaring ``private static final Logger LOGGER = 
Logger.getLogger(MyClass.class)``, declare it as ``private static final Logger 
LOGGER = Logger.getLogger(CONSTANT_WITH_THE_NAME_OF_ COMMON_LOGGER)``).

As I mentioned in a previous comment, these type of compiler gymnastics are 
not necessary to test log output.  Log4j provides the means to attach a custom 
appender to the logger so that you can monitor and test output.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-210018309
  
@syed, I totally understand that you do not have much experience with Java 
development and that you were following the practices already put in place. The 
problem is that we have so many cases of what not to do. I find it very good 
that you are willing to learn and help us improve the ACS code base.

So let’s go to the work.

Sure I changed the variable “logger” to protected, I needed to access 
it on a test case, so I changed it to an access delimiter that I could use; you 
should always try to use the most restrictive one, but at the end in Java those 
access delimiters can always be bypassed using Reflections. The protected 
delimiter states that children’s of that class and classes at the same 
package will have access to that variable/method.

I also removed the “final” because I needed to change that variable to 
the test with a mocked one. The final delimiter would not allow that; 
therefore, I had to remove it. That is why I would normally avoid the use of 
final everywhere, it can complicate things. I would only use final where I 
really want to be strict and not allow a variable change on the fly.

I removed the static to transform that variable into an “instance” 
attribute; those objects are spring beans, and they are working as singletons, 
there will be no more than one instance of them. That is why I see no reason to 
use that attribute as a class field.
Now, you could go over all of the classes that extend 
“NfsSecondaryStorageResource”, they would have their own “logger” 
variable, then you could remove the “s_logger” they are using and use the 
one inherited. Additionally, you would be using a more common name for that 
variable that is “logger”. 

For your second batch of questions, I prefer to use the most restrictive 
delimiter access possible. I always start with private and then I go opening 
them on a need basis. I have no idea why someone would do different, such as 
using “public static final” when it is not needed. ACS has a lot of code 
that should not be taken as reference. In doubt, you can always call for help; 
there are folks here that are great devs.






---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-209759974
  
For sure @rafaelweingartner. As I have outlined in my mail to you, I've 
been basically following the practices that have been put in place by the 
previous developers. If they are not the most optimal/best ways to do things, I 
am willing to learn the right way. 

I've seen your PR. You've made the logger a `protected` variable instead of 
a `static final` Can you tell me the difference? Is one preferred over the 
other? In all the code that I've seen on cloudstack, the `s_logger` is `public 
static final`. Do you have knowldege of why it was done so? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-209435788
  
@syed I have seen the code, and I believe we could work a little bit here. 
Are you willing to?

For instance, class "NfsSecondaryStorageResource" lines 757-768 could be 
extracted to a method. Then, we can also write a test case to that method to 
check if the content that is written as expected. The code at lines 757-768 is 
the same as the one at lines 952-963.

After you extract that I have some other points for improvements, but 
let’s walk a step at time.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-209431789
  
@syed sorry the delay, I just read your email asking for some assistance; 
it is the time zone :(

I have opened a PR to your branch with the test case for the 
"cleanupStagingNfs" method.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-209390353
  
@rafaelweingartner I've written a simple test which will fail if someone 
throws an exception and it is not caught. I've tried mocking the `s_logger` but 
for it to work is not easy. I've sent you a mail explaning the situation. In 
any case, even if someone decides to silence exceptions I don't really care as 
I am not doing anything with them anyways. 

Let me know what you think. 




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-208318739
  
@syed, you can change the method to be protected.
IMO every single method that has some logic in it should be tested without 
any discrimination among private and non-private ones.

To write a test for this method, you can use Mockito; you can use the spy 
method into the object that you want to test (NfsSecondaryStorageResource), 
then you can force an exception to be thrown when the "execute" method is 
called with “deleteCommand” object.
To check if the exception was treated properly you can use Mockito to mock 
the s_logger object and check if the “debug” method was called after the 
exception is thrown

This way, if someone changes the logic and let the exception be re-thrown, 
the test case will catch. Also, if someone silences the exception, the test 
case will catch.

If you have doubts/problems when writing the test case, just call me.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-208176655
  
@rafaelweingartner Updated the comment as a javadoc.

For the test. I am not sure what the best strategy is. Maybe you can help 
me understand better. Would would be the best stragetgy for testing private 
methods. For me this method should be private and I don't know how the 
community feels like testing private methods.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-208034027
  
@syed, you could delete the line 631, or use it as a java doc that explains 
the method.

I believe that there is, at least, one test you can do here. You could test 
if an exception happens while calling the “execute” method with the 
“deleteCommand” variable. If an exception happens it cannot be re-thrown by 
this method, and it has to be logged as a debug message.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-207898130
  
@rafaelweingartner Done. Although I didn't create any test cases yet. There 
is nothing much to test. The function doesn't throw any execptions, doesn't 
expect any results so I think this should be good. 

@swill I've rebased again so it is still a single commit



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-207676683
  
Hi, would you mind extracting to a method lines 606-612? They are 
duplicated at line 627.
Then, you could even create test cases for it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-207676112
  
@swill I've squashed the commits 

@kollyma I'm sorry I missed your message. Do you still see a problem? 
Something strage that I see in your output is the 
`size=1753481216c22-9799-3cc9b79cb771` are you sure this is the case?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-207555946
  
@syed can you squash the commits please and do a `push -f` to update the PR 
branch?

Do you have any feedback for @kollyma if he is seeing a similar issue as 
what you fixed here for swift?  it would be worth understanding if the problem 
exists in more than one place so we can better understand if we need another PR 
for NFS storage as well.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[kollyma]

Github user kollyma commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-202547429
  
@syed: thanks for the feedback. Here a more detailed description, based on 
your comments. 
Setup: CS4.8, nfs secondary storage, kvm with local storage
```
# cat template.properties 
uniquename=225-2-c7401a18-2163-3c22-9799-3cc9b79cb771
filename=eabd7703-4203-49c5-a3a3-f8234f2d1c23.qcow2
size=1753481216c22-9799-3cc9b79cb771
qcow2.filename=eabd7703-4203-49c5-a3a3-f8234f2d1c23.qcow2
qcow2.virtualsize=21474836480
public=true
id=1
```
Entry on the management server: 
```
 select * from vm_template where name = "myname2" \G;
*** 1. row ***
  id: 225
 unique_name: 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771
name: myname2
uuid: cec0787b-5362-4e29-9ca6-63697ff8d872
  public: 1
featured: 0
type: USER
 hvm: 1
bits: 64
 url: NULL
  format: QCOW2
 created: 2016-03-28 18:36:00
 removed: NULL
  account_id: 2
checksum: NULL
display_text: myname
 enable_password: 0
   enable_sshkey: 0
 guest_os_id: 221
bootable: 1
 prepopulate: 0
 cross_zones: 0
 extractable: 0
 hypervisor_type: KVM
  source_template_id: 215
template_tag: NULL
sort_key: 0
size: 21474836480
   state: Active
update_count: 0
 updated: NULL
dynamically_scalable: 0
```
Logs after management server restart:
```
2016-03-28 21:06:33,660 INFO  [o.a.c.s.i.TemplateServiceImpl] 
(AgentConnectTaskPool-3:ctx-3eb6adca) (logid:9e020fbf) Template Sync did not 
find 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771 on image store 1, may request 
download based on available hypervisor types
2016-03-28 21:06:33,661 INFO  [o.a.c.s.i.TemplateServiceImpl] 
(AgentConnectTaskPool-3:ctx-3eb6adca) (logid:9e020fbf) Removing leftover 
template 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771 entry from template store 
table
2016-03-28 21:06:33,668 INFO  [o.a.c.s.i.TemplateServiceImpl] 
(AgentConnectTaskPool-3:ctx-3eb6adca) (logid:9e020fbf) Skip downloading 
template 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771 since no url is specified.
```
You are referring to the path set incorrectly in template properties. The 
cloudstack logs says "Template Sync did not find..." Do we have the same issue 
? the template file is present on the nfs:

template/tmpl/2/225# ls
eabd7703-4203-49c5-a3a3-f8234f2d1c23.qcow2  template.properties




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-201355723
  
@kollyma This was happening for swift because the `template.properties` 
file was not correctly populated. So, when a management restart happens, it 
syncs data from the secondary storage to see which files are present and which 
files need to be downloaded and it is at this point that the managment server 
sets the templates in the DB to `not ready` state. I'm not sure if this is the 
case with pure NFS but you might want to look at your `template.properties` 
files on the NFS to make sure they are correct. 




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[bvbharatk]

Github user bvbharatk commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-201139141
  
_Link to logs Folder (search by build_no):_ 
https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

### ACS CI BVT Run
 **Sumarry:**
 Build Number 126
 Hypervisor xenserver
 NetworkType Advanced
 Passed=104
 Failed=2
 Skipped=4


**Failed tests:**
* integration.smoke.test_guest_vlan_range.TestDedicateGuestVlanRange

 * test_dedicateGuestVlanRange Failed

* integration.smoke.test_volumes.TestCreateVolume

 * test_06_download_detached_volume Failed


**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso

**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData

integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk

integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[kollyma]

Github user kollyma commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-200386681
  
@syed: Thanks a lot for your fix. We are facing the same issue with CS4.8 
and NFS secondary storage. Templates go into "Not Ready" when restarting the 
management server. Did you had the chance to verify what causes the job to 
fail? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-182023310
  
@DaanHoogland I'm not sure why the tests are failing. My changes shouldn't 
have affected that


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-181507090
  
Fixed



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-180438974
  
@swill @syed please have a look at : 
services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java:941:
 Line has trailing spaces.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-179964783
  
Should Jenkins and CI checks be succeeding or is it because the tests do 
not have access to a Swift instance to test and succeed?  

The code looks good to me.  I have not tested because I do not have access 
to a different Swift installation than the others who have tested already.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[pdube]

Github user pdube commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-179502626
  
Tested on local setup, the template.properties is now uploaded correctly 
LGTM


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[pdion891]

Github user pdion891 commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-179568067
  
tested, I think this should be ported into master as well.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[wido]

Github user wido commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-175825330
  
Code looks good, but I have no way of testing this since I don't have 
Swift...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r50443906
  
--- Diff: 
services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
 ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
 }
 }
 
+protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, 
String containerName) throws IOException {
+
+
+//create a template.properties for Swift with its correct unique 
name
--- End diff --

Thanks for the suggestion. I finally was able to get some time to work on 
this again. I'll send a new PR soon with your comments. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r50446719
  
--- Diff: 
services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
 ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
 }
 }
 
+protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, 
String containerName) throws IOException {
+
+
+//create a template.properties for Swift with its correct unique 
name
+File uniqDir = _storage.createUniqDir();
+String metaFileName = uniqDir.getAbsolutePath() + File.separator + 
"template.properties";
+_storage.create(uniqDir.getAbsolutePath(), "template.properties");
+
+String uniqueName = FilenameUtils.getBaseName(srcFile.getName());
+File metaFile = new File(metaFileName);
+FileWriter writer = new FileWriter(metaFile);
+BufferedWriter bufferWriter = new BufferedWriter(writer);
+bufferWriter.write("uniquename=" + uniqueName);
+bufferWriter.write("\n");
+bufferWriter.write("filename=" + srcFile.getName());
+bufferWriter.write("\n");
+bufferWriter.write("size=" + srcFile.length());
+bufferWriter.write("\n");
+bufferWriter.write("virtualsize=" + getVirtualSize(srcFile, 
getTemplateFormat(srcFile.getName(;
+bufferWriter.close();
+writer.close();
+
+SwiftUtil.putObject(swift, metaFile, containerName, _tmpltpp);
+metaFile.delete();
+uniqDir.delete();
+
+return true;
+}
+
+
+protected Answer copyFromNfsToSwift(CopyCommand cmd) {
+
+final DataTO srcData = cmd.getSrcTO();
+final DataTO destData = cmd.getDestTO();
+
+DataStoreTO srcDataStore = srcData.getDataStore();
+NfsTO srcStore = (NfsTO)srcDataStore;
+DataStoreTO destDataStore = destData.getDataStore();
+File srcFile = getFile(srcData.getPath(), srcStore.getUrl());
+
+SwiftTO swift = (SwiftTO)destDataStore;
+
+try {
+
+String containerName = 
SwiftUtil.getContainerName(destData.getObjectType().toString(), 
destData.getId());
+String swiftPath = SwiftUtil.putObject(swift, srcFile, 
containerName, srcFile.getName());
+
+
+DataTO retObj = null;
+if (destData.getObjectType() == DataObjectType.TEMPLATE) {
+swiftUploadMetadataFile(swift, srcFile, containerName);
+TemplateObjectTO newTemplate = new TemplateObjectTO();
+newTemplate.setPath(swiftPath);
+newTemplate.setSize(getVirtualSize(srcFile, 
getTemplateFormat(srcFile.getName(;
+newTemplate.setPhysicalSize(srcFile.length());
+
newTemplate.setFormat(getTemplateFormat(srcFile.getName()));
+retObj = newTemplate;
+} else if (destData.getObjectType() == DataObjectType.VOLUME) {
+VolumeObjectTO newVol = new VolumeObjectTO();
+newVol.setPath(containerName);
+newVol.setSize(srcFile.length());
--- End diff --

Good catch @pdube. That should be the virtual size (not the physical) 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

Github user syed commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-173678575
  
Jira ticket : 
[CLOUDSTACK-9247](https://issues.apache.org/jira/browse/CLOUDSTACK-9247)

I've tested this on our local setup and it works as expected. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[pdube]

Github user pdube commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r50066929
  
--- Diff: 
services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
 ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
 }
 }
 
+protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, 
String containerName) throws IOException {
+
+
+//create a template.properties for Swift with its correct unique 
name
+File uniqDir = _storage.createUniqDir();
+String metaFileName = uniqDir.getAbsolutePath() + File.separator + 
"template.properties";
+_storage.create(uniqDir.getAbsolutePath(), "template.properties");
+
+String uniqueName = FilenameUtils.getBaseName(srcFile.getName());
+File metaFile = new File(metaFileName);
+FileWriter writer = new FileWriter(metaFile);
+BufferedWriter bufferWriter = new BufferedWriter(writer);
+bufferWriter.write("uniquename=" + uniqueName);
+bufferWriter.write("\n");
+bufferWriter.write("filename=" + srcFile.getName());
+bufferWriter.write("\n");
+bufferWriter.write("size=" + srcFile.length());
+bufferWriter.write("\n");
+bufferWriter.write("virtualsize=" + getVirtualSize(srcFile, 
getTemplateFormat(srcFile.getName(;
+bufferWriter.close();
+writer.close();
+
+SwiftUtil.putObject(swift, metaFile, containerName, _tmpltpp);
+metaFile.delete();
+uniqDir.delete();
+
+return true;
+}
+
+
+protected Answer copyFromNfsToSwift(CopyCommand cmd) {
+
+final DataTO srcData = cmd.getSrcTO();
+final DataTO destData = cmd.getDestTO();
+
+DataStoreTO srcDataStore = srcData.getDataStore();
+NfsTO srcStore = (NfsTO)srcDataStore;
+DataStoreTO destDataStore = destData.getDataStore();
+File srcFile = getFile(srcData.getPath(), srcStore.getUrl());
+
+SwiftTO swift = (SwiftTO)destDataStore;
+
+try {
+
+String containerName = 
SwiftUtil.getContainerName(destData.getObjectType().toString(), 
destData.getId());
+String swiftPath = SwiftUtil.putObject(swift, srcFile, 
containerName, srcFile.getName());
+
+
+DataTO retObj = null;
+if (destData.getObjectType() == DataObjectType.TEMPLATE) {
+swiftUploadMetadataFile(swift, srcFile, containerName);
+TemplateObjectTO newTemplate = new TemplateObjectTO();
+newTemplate.setPath(swiftPath);
+newTemplate.setSize(getVirtualSize(srcFile, 
getTemplateFormat(srcFile.getName(;
+newTemplate.setPhysicalSize(srcFile.length());
+
newTemplate.setFormat(getTemplateFormat(srcFile.getName()));
+retObj = newTemplate;
+} else if (destData.getObjectType() == DataObjectType.VOLUME) {
+VolumeObjectTO newVol = new VolumeObjectTO();
+newVol.setPath(containerName);
+newVol.setSize(srcFile.length());
--- End diff --

Not 100% related to this PR, but are the sizes on volumes and snapshots 
correct?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[pdube]

Github user pdube commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-172715155
  
Code LGTM, as a general comment though, I think it is cleaner to be as 
precise as possible with exception handling.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[cristofolini]

Github user cristofolini commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-172214758
  
@DaanHoogland That's a really interesting feature which I wasn't aware of. 
I'm afraid I'll be out of commission for most of today but I'd love to try it 
out as soon as I can get my hands on a computer. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[cristofolini]

Github user cristofolini commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-172240147
  
@DaanHoogland @syed I've opened a pull request containing some simple 
javadocs to @syed's swift-restart-fix branch.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[cristofolini]

Github user cristofolini commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1331#discussion_r49888184
  
--- Diff: 
services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
 ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
 }
 }
 
+protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, 
String containerName) throws IOException {
+
+
+//create a template.properties for Swift with its correct unique 
name
--- End diff --

Since you went through the trouble of cleaning up some of the code, how 
about you improve it some more by turning this comment into a javadoc comment 
for this method, also describing the parameters involved. 

If you like this idea, I'd suggest going ahead and writing another javadoc 
comment for the `copyFromNfsToSwift` method.

Little things like these can go a long way to improve code readability. :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1331#issuecomment-171276640
  
@syed thanks for picking this up. Can you 
- link this to a ticket
- describe any testing you did (I can't test this)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[syed]

GitHub user syed opened a pull request:

https://github.com/apache/cloudstack/pull/1331

Fix Sync of template.properties in Swift

When using Swift as a secondary storage, we create a template.properties 
file which stores the metadata about the template. Currently the data which is 
present in the file is incorrect which leads to templates becoming unavailable 
after they are downloaded. This fix makes sure that the template.properties has 
the correct "path" set so that templates are available. 

I've also done a bit of cleanup and made the code bit more clean. 


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/syed/cloudstack swift-restart-fix

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1331.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1331


commit 1b9ff622f2b9a38e65e31c7f803fd8d283f5d36f
Author: Syed 
Date:   2015-12-14T22:37:28Z

Fix Sync of template.properties in Swift




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---