Changed variable s_logger to non-static and fixed its name in “com.cloud.utils.component.ComponentLifecycleBase” and its subclasses (CLOUDSTACK-8750)

[rafaelweingartner]

Re-pushing PR 714 (#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.

[asfbot]

cloudstack-pull-rats #507 SUCCESS
This pull request looks good

[asfbot]

cloudstack-pull-analysis #440 FAILURE
Looks like there's a problem with this pull request

[rafaelweingartner]

Something seems to have crashed the JVM during Jenkins build; can someone with access to the server get some extra log files? I have built it here without any problems.

[remibergsma]

@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!

[asfbot]

cloudstack-pull-rats #509 SUCCESS
This pull request looks good

[asfbot]

cloudstack-pull-analysis #442 SUCCESS
This pull request looks good

[remibergsma]

@karuturi Can you have a look please and maybe check the noredist build?

[asfbot]

cloudstack-pull-rats #579 ABORTED

[asfbot]

cloudstack-pull-analysis #515 ABORTED

[wilderrodrigues]

@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

[miguelaferreira]

@rafaelweingartner do you think "Re-pushing the PR 714" is a good commit header?
I mean, when someone goes through the commit list looking for something, will that header provide information to what is the underlying code change about?

[rafaelweingartner]

@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 ;)

[miguelaferreira]

@rafaelweingartner I totally understand. Inspiration is not something that always comes when needed. Please do take your time.

[rafaelweingartner]

@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

[asfbot]

cloudstack-pull-rats #608 SUCCESS
This pull request looks good

[asfbot]

cloudstack-pull-rats #611 SUCCESS
This pull request looks good

[asfbot]

cloudstack-pull-analysis #543 SUCCESS
This pull request looks good

[asfbot]

cloudstack-pull-analysis #546 SUCCESS
This pull request looks good

[miguelaferreira]

@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?

[rafaelweingartner]

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.

[miguelaferreira]

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 how they are declared and used seems 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.

[rafaelweingartner]

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.

[wilderrodrigues]

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

[wilderrodrigues]

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 👎 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

[rafaelweingartner]

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.

[wilderrodrigues]

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.
• ACS is used by many companies, small and big ones. e have to take good care of it.
• 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. ;)

[rafaelweingartner]

@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 ;)