Changed variable s_logger to non-static and fixed its name in “com.cloud.utils.component.ComponentLifecycleBase” and its subclasses #714
Conversation
|
cloudstack-pull-analysis #278 FAILURE |
|
cloudstack-pull-rats #346 FAILURE |
|
cloudstack-pull-requests #1041 FAILURE |
|
@pedro-martins s_logger is a standard name for the variables and in each class it gets the .class as its log category. I fail to see how you change improves on this. |
pedro-martins
force-pushed
the
master-lrg-cs-hackday-003
branch
from
631dcca to
e384f2d
Compare
|
@DaanHoogland I agree with your considerations. My first commit was not performed properly. Sadly, the eclipse ended up formatting classes I touched, and that is why the line number added was higher than the removed one. I do understand that “s_ something“ is a proper way to instantiate a static variable in ACS classes. However, in few of the subclasses of “com.cloud.utils.component.ComponentLifecycleBase” it was used names such as “LOGGER” (that is a proper name for static field as JAVA standards), log, status_logger and others. What we propose is to change the logger variable in “com.cloud.utils.component.ComponentLifecycleBase” to protected and remove its static and final declaration. Therefore we did the following in ComponentLifecycleBase: I have edited the commit, what do you think of it now? |
|
@pedro-martins your motivation makes sense to me, can you create a jira ticket and c&p it there? then use that as a tag for any commits (including cleanup/autoformat). One concern remaining is (not knowing the entire hierarchy by head) whether instance level loggers will be any kind of overhead. I know there are a lot of singletons per management server instance but for for instance agents this is not true. Did you investigate this? |
|
cloudstack-pull-rats #354 ABORTED |
|
Got your comments, sadly we have not analyzed which classes are singleton or not. |
|
cloudstack-pull-analysis #286 SUCCESS |
|
Hi @DaanHoogland, after we opened the Jira Ticket, someone ended up assigning it to us!? So, we decided to take a lead and analyze the impact that the proposed changes may cause. It turns out that most of the classes of that hierarchic are singletons, therefore it would not have an impact on memory consumption. The result of the analysis is the following: Beans instantiated with @component:
Beans instantiated in Spring XMLs:
Abstract classes:
Spring beans instantiated in XML and using @Local (EJB) annotation ?!!?:
This class is never instantiated: /cloudstack/plugins/storage-allocators/random/src/org/apache/cloudstack/storage/allocator/RandomStoragePoolAllocator.java Others: Instantiated only once at: org.apache.cloudstack.storage.template.DownloadManagerImpl.configure(String, Map<String, Object>)
Only used in tests:
Never used:
Instantiated when adding a new DHCP server
Used only when adding a NuageVspDevice Instantiated when adding pxeserver Instantiated when adding baremetal cluster Instantiated when adding BigSwitchBcf device Just used by Globo when adding a dns server Only used in test cases!? Instantiated in a few different cases: That was the full report. If you give the go ahead we can get the head revision execute our scripts and commit the changes. Would you re-open the PR? PS: We found a little odd some spring beans with @Local annotation, we intend to investigate that later. Sorry the huge post. |
|
cloudstack-pull-rats #366 FAILURE |
|
cloudstack-pull-analysis #299 FAILURE |
|
cloudstack-pull-rats #367 FAILURE |
|
cloudstack-pull-analysis #300 FAILURE |
rafaelweingartner
force-pushed
the
master-lrg-cs-hackday-003
branch
from
ddcd1ed to
aea9d4b
Compare
|
cloudstack-pull-rats #368 SUCCESS |
|
cloudstack-pull-rats #369 SUCCESS |
|
cloudstack-pull-rats #370 FAILURE |
rafaelweingartner
force-pushed
the
master-lrg-cs-hackday-003
branch
from
802aa6c to
9493d0b
Compare
|
cloudstack-pull-rats #371 FAILURE |
|
cloudstack-pull-analysis #301 SUCCESS |
|
cloudstack-pull-analysis #302 SUCCESS |
|
cloudstack-pull-analysis #303 UNSTABLE |
|
cloudstack-pull-analysis #304 UNSTABLE |
|
cloudstack-pull-rats #372 FAILURE |
|
cloudstack-pull-analysis #305 SUCCESS |
|
As the system won't be build with just the first commit applied can you squash those two commits? |
|
cloudstack-pull-rats #443 SUCCESS |
|
cloudstack-pull-analysis #376 SUCCESS |
|
LGTM |
|
travis timeouts are unrelated. analysis passes, LGTM |
|
Thanks for the hard work on reviewing this PR |
Changed variable s_logger to non-static and fixed its name in com.cloud.utils.component.ComponentLifecycleBase and its subclassesHi guys, We have noticed that every single class that is a subclass of ComponentLifecycleBase instantiate their on logger manually and uses a nonstandard name. We fixed that by changing the variable in ComponentLifecycleBase to protected and non-static and instantiated it using the method getClass from Object class. Therefore, we can reduce the code in a few hundred lines and use a more intuitive name for the logger variable. During that process we found a static method that used the s_logger variable in classes: com.cloud.network.element.VirtualRouterElement org.apache.cloudstack.network.element.InternalLoadBalancerElement To fix that we had to create a new class com.cloud.network.element.HAProxyLBRule, instantiate it with @componente and inject into the aforementioned classes. The class that we create is com.cloud.network.element.HAProxyLBRule and has the following methods: com.cloud.network.element.HAProxyLBRule.containsOnlyNumbers(String, String) com.cloud.network.element.HAProxyLBRule.validateHAProxyLBRule(LoadBalancingRule) Sadly we could not write test cases to it; hence we did not fully understand those methods. However, if anyone out there understands it, we would appreciate some code to be added to it. As minor this change may seem; we believe that it enhances a little bit the ACS code by using standard name to logger variable. * pr/714: Solved jira ticket: CLOUDSTACK-8750 Signed-off-by: Daan Hoogland <[email protected]>
|
jenkins noredist build failed with the below error. Reverting this PR http://jenkins.buildacloud.org/job/build-master-noredist/4588/consoleText |
…hackday-003" This reverts commit cd7218e, reversing changes made to f5a7395. Reason for Revert: noredist build failed with the below error: [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.2:compile (default-compile) on project cloud-plugin-hypervisor-vmware: Compilation failure [ERROR] /home/jenkins/acs/workspace/build-master-noredist/plugins/hypervisors/vmware/src/com/cloud/hypervisor/guru/VMwareGuru.java:[484,12] error: non-static variable logger cannot be referenced from a static context [ERROR] -> [Help 1] even the normal build is broken as reported by @koushik-das on dev list http://markmail.org/message/nngimssuzkj5gpbz
|
Hi @karuturi, If you take a look at here: |
|
@karuturi, I was just reviewing my commit, I am really sorry. |
|
@mike-tutkowski @DaanHoogland @karuturi @miguelaferreira @rafaelweingartner @pedro-martins @remibergsma First of all let me just get one thing clear: Rafael mentioned that he has a script that changes the Java files automatically. Did I read it right? If so, why doesn't this script also compile the code with a simple Maven run after the changes? We have been suffering from things that simply break and are difficult to track because PRs are not properly reviewed/tested. In this particular case, how is possible to get 2 LGTM without even compiling the project locally? Cheers, |
|
@wilderrodrigues I commented: "travis timeouts are unrelated. analysis passes, LGTM". So it was compiled otherwise the analysis would not have passed. What I missed there was the noredist build. This one must be run instead of the regular in cloudstack-pull-anaysis. hat job doesn't run at all atm. I made an infra ticket for that and next we need to (finaly) get the noredist in there. It had been on my list and was phased out of it. it is no back on :( |
|
Hi @DaanHoogland, what I don't understand is how travis compiled and 15 days ago @rafaelweingartner said that he forgot to push 1 file. Looking at the history, we see the following:
So, perhaps the timeout occurred before the missing file was needed, so one could not tell if it would have worked at all. Therefore we have to compile the PRs and test them before LGTM. I also don't get why @mike-tutkowski simply gave a LGTM without any comment. Come on, guys... lets improve ACS, not make it worse. In the past - not long ago - we had problems with HyperV, not even used in our environment, and also with static fields not initialised properly that caused systemVMs to not start at all. If I see a change of +4k line 380 files in a tightly-coupled system like ACS, I would test it as much as I could before saying anything. @rafaelweingartner @pedro-martins : do you have a test environment? Cheers, |
|
and later we see:
|
|
@wilderrodrigues I got your point, and I also hate when projects that I work start breaking as a consequence of bad/poor code. Here goes an explanation in a timeline manner, so you can get a better understanding of everything that happened: When I assigned that “task” to an intern (@pedro-martins), he executed it going class by class literally (I have to give credits for him to be so calm and controlled, I would not stand to do that). While doing that he noted down classes that he was changing the code. When he pushed, @DaanHoogland pointed out that the PR was adding more lines then removing due to code formatting. Therefore, he created a script that was fed with that list of classes and performed the changes (deleted the lines that had to be deleted and changed the variable names that had to be changed). Another problem arose, for some reason in a few classes such as “VMwareGuru” there are static methods that do not need to be static and that use the “logger” variable, with our PR that had to be changed. He was supposed to find each of the classes that had a problem from that “ComponentLifecycleBase” hierarchic and fix it. Sadly, he missed one class out of 380. We indeed tried to build the whole project with a maven install, but it did not work at that time, there was a problem with a class that we have not touched. After that the PR 714 was merged by @DaanHoogland, and then reverted by @karuturi. When I noticed that problem, I got the task and created a script that really builds the whole “ComponentLifecycleBase” hierarchic and executes the deletes and changes on variable names that are needed. Then I manually check the classes that have some problem, around 8 classes that have static methods that do not need to be static. I created a new PR #778 for that, because we did not find a way to re-open the PR #714. The first time I pushed to PR 778 I successfully did a maven install on ACS to check for compilation problems that might be caused by our change. Then the time passed on (conflicts appeared) and I did another two rebases, the first one I hit a problem with a missing class on “cloud-plugin-network-vcs” and the last one another missing class on “cloud-framework-db”. I had sent an email asking about the first missing class I found, but got no answers. Now to answer your first question, I indeed created a script, but just after the PR 714 was reverted (we were not kidding about that). Moreover, the pushes I did to PR 778 I made sure to execute a maven run to build the project and see if everything was ok. Sadly I hit other problems with missing classes. Sadly during PR 714, we had problems with Travis build timeouts at the same time that we were working on PR 714, if Travis CI was working properly we would have spotted the problem and the PR 714 would not need to be reverted. @wilderrodrigues now your last point, we indeed have a production and test environments running ACS 4.3.2 (both the same version because we are extending ACS, everything we build is added into our 4.3.2, tested and then pushed to our production environment), that is why we are not building the components changed and testing it here. We did not see a reason to add this PR was not also added to 4.3.2. |
|
"In this particular case, how is possible to get 2 LGTM without even compiling the project locally?" I was depending on the info Daan refers to and, as such, did not build it locally. By the time I came into the picture with regards to this PR, it was mainly a matter of looking through 361 files for changes to the name of a variable. Not sure how verbose of a comment you'd like for that, but it looked reasonable to me. Perhaps we need to document on our Wiki exactly what needs to go into a review (including comments). A lot of reviews for PRs that I've seen have taken my approach here: The PR submitter asked for another review, a reviewer took time to go through the code online, but didn't compile because he/she expected asfbot's SUCCESS meant it was OK from that standpoint, then wrote "LGTM." |
|
I could say this again, but it doesn't seem like it's a popular opinion with regards to CloudStack development: If we are really "late in the game" with regards to the 4.6 release, then probably half or more of the PRs we put into the codebase should not be going in at this point (they should be going into 4.7). The way most software development I've been involved with over the past 17 years works is that defect and enhancement tickets are prioritized by a cross-functional team (it doesn't have to be cross functional, though, for CS). These defect/enhancement priorities along with the stage at which we are in development (i.e. how close we are to our release deadline) inform what actually goes into the current release and what is postponed to the next release. We have a habit with CS of just continuously churning the codebase up until the very last moment. If we had an awesome suite of regression tests, we might be able to get away with that. I say "we might," but - in reality - it's still not a good idea to do that. Many defects don't show up until the code has "soaked" over long periods of time and we don't have any way to protect against that the way we develop and test CS code. |
|
@mike-tutkowski we don't have an awesome suite of regression tests because we don't write them. And if so, we should not rely on the builds we have, like @DaanHoogland mentioned, because we know they are flakey at best. I do agree with you when you say that we should make the review process a bit more detailed and formalised. Simply giving out a LGTM because someone else already did and we don't see anything strange while skimming through the code, is definitely not enough. |
|
Right, Miguel (as you and I were discussing for that other PR with regards to tests). My point then is we especially should not be putting in this much code so close to a 4.6 GA. No commercial-grade software I've ever worked on does that (and they did have awesome suites of regression tests). |
|
I completely agree with you in this Mike. We should stop churning in features when we don't even have a proper build that prevents syntax errors! |
|
Maybe the compiler is overworked and just did a cursory glance and reported back "LGTM." ;) |
|
Hi @rafaelweingartner , I understood that @pedro-martins went through a hard time checking class by class, but I would not discuss credits and that kind of stuff here because it could sound like an excuse. Being pragmatic: the PR is huge, lacks unit/integration tests and did not even compile. We are all here to help and understand how to get all software/system engineers to commit code that keeps us, at least, in the same quality level we have now: not decreasing our already poor coverage. Concerning your environment: nice you have 4.3.2 in production and test. However, if you are changing master - 4k lines - it would be extremely nice to have a test environment to cover your changes. If building a new test environment would cost too much time, you could at least have something like:
component/test_routers.py All instructions about how to run tests and the simulator can be found here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Validating+check-ins+for+your+local+changes%2C+using+Simulator If you want to build a DevCloud environment and use it to execute more advanced integration tests against, please check here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/DevCloud We are here to help. Cheers, |
|
I will add the same comment to the opened PR instead. I believe is better to follow up overt here. |
…-006 Removed unnecessary @Local annotations and their respective importsFollowing @rafaelweingartner 's findings in PR #714 that many spring beans contained an @Local annotation, we've decided to remove said annotations and their imports from the ComponentLifecycleBase class and its subclasses seeking a reduction of a few hundred lines of useless code. I had already opened a pull request for this (#853) but at some point my commit disappeared from the PR entirely, showing no new changes in code, which caused it to be merged automatically (with no changes). * pr/1102: Removed unnecessary @Local annotations and their respective imports from the ComponentLifecycleBase class and its subclasses. Signed-off-by: Remi Bergsma <[email protected]>
Customize link hover color Customize loading color Customize navigation menu color Fixes #712 Fixes #713 Fixes #714 Signed-off-by: Rohit Yadav <[email protected]>
9 participants
Hi guys,
We have noticed that every single class that is a subclass of “ComponentLifecycleBase” instantiate their on “logger” manually and uses a nonstandard name. We fixed that by changing the variable in “ComponentLifecycleBase” to protected and non-static and instantiated it using the method “getClass” from Object class. Therefore, we can reduce the code in a few hundred lines and use a more intuitive name for the logger variable.
During that process we found a static method that used the “s_logger” variable in classes:
com.cloud.network.element.VirtualRouterElement
org.apache.cloudstack.network.element.InternalLoadBalancerElement
To fix that we had to create a new class “com.cloud.network.element.HAProxyLBRule”, instantiate it with @componente and inject into the aforementioned classes.
The class that we create is “com.cloud.network.element.HAProxyLBRule” and has the following methods:
com.cloud.network.element.HAProxyLBRule.containsOnlyNumbers(String, String)
com.cloud.network.element.HAProxyLBRule.validateHAProxyLBRule(LoadBalancingRule)
Sadly we could not write test cases to it; hence we did not fully understand those methods. However, if anyone out there understands it, we would appreciate some code to be added to it.
As minor this change may seem; we believe that it enhances a little bit the ACS code by using standard name to logger variable.