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
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
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
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,
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
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
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
```
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
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.
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]
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
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.
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
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
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
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
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 @@
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 @@
+/*
+*
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 @@
+/*
+*
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
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
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
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
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
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`
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,
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
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
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``
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
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
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
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
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.
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"
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
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
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
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
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
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
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
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
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
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
```
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
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
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
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
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
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
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
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
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
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
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
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
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
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 @@
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
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
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
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 @@
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
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
65 matches
Mail list logo
