You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by rafaelweingartner <gi...@git.apache.org> on 2015/09/04 20:47:37 UTC
[GitHub] cloudstack pull request: Jira ticket CLOUDSTACK-8750
GitHub user rafaelweingartner opened a pull request:
https://github.com/apache/cloudstack/pull/778
Jira ticket CLOUDSTACK-8750
Re-pushing PR 714 (https://github.com/apache/cloudstack/pull/714). We had forgotten a class behind which generated a blocker issue on master (my bad, sorry the trouble). Therefore, it had to be reverted. This PR is meant to reintroduce the changes that were coded there (PR 714). This way we can close the Jira ticket: CLOUDSTACK-8750.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rafaelweingartner/cloudstack master-lrg-cs-hackday-003
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/778.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 #778
----
commit 8a83968d5ec8cfa4c75251021cf48a72e01ed7c7
Author: Rafael Weingartner <ra...@gmail.com>
Date: 2015-09-04T18:33:20Z
Jira ticket CLOUDSTACK-8750
Re-pushing the PR 714, we had forgotten a class behind and it had to be
reverted.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140487166
@rafaelweingartner Thanks for the time you spent on this. The commit header is indeed better. It now conveys what is being done. If you happen to have to change something in the commit again (for other reasons), may I suggest you follow what most people do and make the header like this: `CLOUDSTACK-8750: changed variable s_logger to logger non-static`
I've posted on the Jira ticket (CLOUDSTACK-8750) a question about why is this change being made. After reading the description of the ticket I couldn't figure out what is the improvement. Maybe you could shed some light on that?
Is it just, not having an extra line in each class declaring the logger? Or am I missing something?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140493836
I do agree that prepending "s_" to static variables is a waste of characters. Modern IDEs will signal static variables for you. It's a bit like prepending (or appending) type information in the variable name. There was a time it was useful, but that time is long gone.
I also appreciate abstraction in code, that is, you've now introduced a protected variable in a base class that will be available to all the classes that extend it. However I do not see a real benefit in adding this particular indirection level. I mean, loggers are very straight forward to add and understand, so standardising on they declaration and use, seem to me far better than having it be a static variable in some classes, and a non static-protected-inherited variable in others.
There is plenty I would like see done in terms of cleaning the code of ACS. For instance I've measured duplicate code a year ago and found 25% of the code to be redundant.
(http://www.slideshare.net/miguel_f/20141121-cccenoanimations)
The DB layer is another example. ACS is at the moment using a deprecated library for it's DB layer, while there are other libraries (e.g. spring-data) that would actually generate most of the hand-made code we have now.
I'll be very happy to share my thoughts with you about potential refactorings that would benefit the ACS 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Jira ticket CLOUDSTACK-8750
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-137835347
Something seems to have crashed the JVM during Jenkins build; can someone with access to the server get some extra log files?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140403964
@rafaelweingartner I totally understand. Inspiration is not something that always comes when needed. Please do take your 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140491359
Actually, the ticket is as simple as that.
It was just an exercise to an intern that is working with me in my thesis. We are doing few very small cleans and changes in ACS code to teach some coding skills to some interns. Thus, we found pretty bothering to code sometimes, we were coding you start to write “log” and hit ctrl+space bar and nothing showed up, the same happens in classes that use the “_” to start variable names. We know that s_something is an ACS standard for static variables, but we thought that the name logger would be better.
That was a pretty interesting proposal, because we could get to know a little better the ACS code, and found some others small things we could change to make the coding more pleasant. For instance, we are planning a PR removing the “@Local” from all classes of ComponenteLifeCycleBase, hence they all are either spring beans or manually instantiated.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140729409
Dear @wilderrodrigues, As I said I get your point and the moment that you guys are on closing a version.
A mistake was made, the moment I noticed that I jumped in and checked the problem and tried to fix it with this PR. No need to cut someone’s head with adamantium claws.
Sure the task was huge, but simple. That was the point on giving it to a less experienced student. I want them to see that even in huge systems like CloudStack and others; there are codes that are not pretty and shiny.
The improvement factor depends on the concept you use, removing few lines of code and using a more intuitive and standard name for a variable in a hierarchic of classes can be considered an improvement. The same occur if we remove the @Local annotation of that hierarchic, the task will affect 600+ classes, but it is a simple one.
Since you said that there is no way this PR will get accepted, I am closing it.
Thanks for your 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Jira ticket CLOUDSTACK-8750
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-137924348
@rafaelweingartner if you force push the commits again, the tests will be started again. Might be a temp issue.
Also, could you update the title of the PR to reflect what is changed? Something like "Change the variable s_logger to non-static and fixed its name" or similar. That way we can more easily see what the change was about.
Did you try a non-redist build? If so, please post result.
Finally, it seems there is a conflict now. Could you rebase against latest master please?
Thanks!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140366106
@karuturi @remibergsma @rafaelweingartner @DaanHoogland @miguelaferreira
Please, before proceeding with this PR see my comments on the previous (#714) one.
We would like to hear what is the problem with the current approach before we get this change in. And before LGTM it, please make sure that at least the following tests are executed:
test_routers.py
test_vm_life_cycle.py
test_vpc_routers.py
test_vpc_routers_nics.py
test_reset_vm_on_reboot.py
Also, this branch branch cannot be automatically merged. It has conflicts! It will be a nightmare to get it through.
Cheers,
Wilder
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140380153
@wilderrodrigues thanks for your comments, I have just answered your comments on PR 714. I will work on a rebase today and push it again.
@miguelaferreira, I really do not find that a good name. I was just without a good inspiration to create one ;) What about this one 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-137957748
@karuturi Can you have a look please and maybe check the noredist build?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140366714
@rafaelweingartner do you think "Re-pushing the PR 714" is a good commit header?
I mean when someone goes thought the commit list looking for something, will that header provide information to what is the underlying code change about?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-141142775
@wilderrodrigues,
Of course no hard feelings.
I understand the pain that you feel when you stabilize everything and people come and breaking things.
About our PR, we are starting to investigate that hierarchic, we believe that some classes do not need to be there. Therefore, it would enable us to shorter the hierarchic and de-couple a little bit the code.
See you in the next PR ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140527418
I do agree with you that removing duplicated code, reducing cyclomatic complexity and writing test case is much better. However, those tasks require certain programing skills and knowledge of the code. I have interns that have never worked with Java, spring*, web applications and Eclipse/Netbeans. Therefore, I am starting introducing simple and pretty easy tasks, so they can get comfortable working on a huge project such as ACS. It takes a while to get used to working with tools such as Eclipse, Maven and Git.
We are at the very beginning of my thesis. Most of them are not ready to start writing TDDs and refactoring methods. The ones that are, we already did some pretty interesting PRs: #560, #700 and #762.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140648456
Hi @rafaelweingartner ,
Master was broken yesterday when a file containing the wrong imports was merged. I reverted about 5 minutes after the merge. Unfortunately, it seems you got your rebase just in between, which caused your maven build to fail. Please rebase again and try it once more.
Please, check my comment on PR #714
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140658135
All,
Given the current blocker issues we have to fix on master in order to release 4.6.0 and the fact that those changes don't really improve the current code base, I will :-1: this PR and will object if this PR gets 2 LGTMs.
@rafaelweingartner, please have a look at the dev-list in order to see the proposal for PRs and how they should be created.
In addition, for your students with less experience with Java/Eclipse/Maven/Git, try to avoid giving them such a huge task.
Cheers,
Wilder
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner closed the pull request at:
https://github.com/apache/cloudstack/pull/778
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-141135958
Hi there @rafaelweingartner,
No hard feelings, dude.
Long story short:
* we are going through a critical phase with ACS - release
* I have been working on ACS since 2013, and it's always the same. We stable master, we have a release and after that master is screwed again. After 6 months we go through this infinite loop of mess and nightmares.
* ACS is super uber tightly coupled, by the [bad] design it has. If I would tell you that we had production problems because of 1 class of 1 hypervisor, that we don't even use, was f*dup, you wouldn't believe me. So, any simple change has the potential to make a huge mess. That's why the tests, etc.
* I respect your efforts and also the guys working with you. I will be an student till I die!
If you can find a way to apply/submit your changes in an progressive way, I'm more than glad to help.
Cheers,
Wilder
P.S.: adamantium claws, you know... I got to use them sometimes. ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/778#issuecomment-140423091
@wilderrodrigues conflicts solved, I tried to do a maven install, to run all tests and compile everything, but that did not work, hence there is a class missing "com.cloud.network.schema.showvcs.Output;"
@miguelaferreira I think the commit heading is better 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---