[GitHub] cloudstack pull request: Removed methods from EventBus interface.
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/1260 Removed methods from EventBus interface. Was removed the methods . org.apache.cloudstack.framework.events.EventBus.subscribe(EventTopic, EventSubscriber) . org.apache.cloudstack.framework.events.EventBus.unsubscribe(UUID, EventSubscriber) from interface org.apache.cloudstack.framework.events.EventBus and it respective implementations org.apache.cloudstack.mom.inmemory.InMemoryEventBus, org.apache.cloudstack.mom.kafka.KafkaEventBus and org.apache.cloudstack.mom.rabbitmq.RabbitMQEventBus because these methods are not used in cloudstack classpath, just in some test cases and these tests cases are removed too. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack lrg-cs-hackday-017 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1260.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 #1260 commit 1e646daec230efc651ad4e85099d50176ee26f00 Author: pedro-martins Date: 2015-12-19T17:02:19Z Removed methods from EventBus interface. --- 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: Removed unused variables from class Netwo...
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/1261 Removed unused variables from class NetworkStateListener Removed unused variables from class NetworkStateListener We removed the following variables from "com.cloud.network.NetworkStateListener" . UsageEventDao _usageEventDao . NetworkDao _networkDao and we changed the EventBus s_eventBus variable to private. Also, we change the constructor to not use those variables and apply this change in classes âcom.cloud.network.IpAddressManagerImplâ and âorg.apache.cloudstack.engine.orchestration.NetworkOrchestratorâ You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack lrg-cs-hackday-19 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1261.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 #1261 commit 5d3adb3a2da6aea6fba8f563548de14cdcca7625 Author: pedro-martins Date: 2015-12-19T17:54:26Z Removed unused variables from class NetworkStateListener --- 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: CLOUDSTACK-9180: Optimize concurrent VM d...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1251#issuecomment-170406819 nice =) Can you create a java doc to explain your 'isRouterDeployed()' (even the name explaining itself)? If you can create a test case for only this method it will be more easy to verify if it stop working in future modifications =) Thx. --- 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: CLOUDSTACK-9199: Fixed deployVirtualMachi...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1280#issuecomment-170409736 Nice :) Can you extract the code to a method with a javadoc explaining the code? If you can do a testcase for the method too, it will be apreciated :) Thx :) --- 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: CLOUDSTACK-9180: Optimize concurrent VM d...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1251#issuecomment-170636370 A javadoc is a notation above the method declaration that describes what the method do. javadoc with examples :) http://www.oracle.com/technetwork/articles/java/index-137868.html I saw that you create a test for the deployVirtualRouter that uses the isRouterDeployed(), but if this test don't pass, will be difficult to analyse if the test don't pass because the isRouterDeployed method or another problem. :) Ty. --- 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: CLOUDSTACK-9165 unable to use reserved IP...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1246#issuecomment-172029080 Hi =) What is the difference between network.getCidr() and network.getNetworkCidr() ? And, there is a possibility to you create a method with a simple javadoc and simple testcase instead of using a short if? Ty =) --- 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: CLOUDSTACK-8959: Option to attach the con...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/938#issuecomment-172103324 Hi. There is no problem to set vmData to null? and what about replace this short if to a method with documentation and a little test. And if you extract all that you do to little methods, it will be easier to create test cases and confirm that you do works correctly Ty =) . --- 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: Refactor system VM default network creati...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1360#issuecomment-174210179 Could you replace the logic in your 'if' at lines 673 and 523 "(networks == null || networks.size() == 0)" to CollectionUtils.isEmpty(networks) from org.apache.commons.collections4.CollectionUtils ? That can make the logic a bit more legible. --- 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: CLOUDSTACK-9100: ISO.CREATE/TEMPLATE.CREA...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1157#issuecomment-174213096 @ SudharmaJain why are you passing null to the method that you created (âcreateTemplateAsyncCallBackâ) at line 503? That will cause a null pointer at the first line of the method as you can see at line 598. Additionally, what about the addition of a test case to this new âcreateTemplateAsyncCallBackâ and a Javadoc to explain what your method does. --- 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: Refactor system VM default network creati...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1360#issuecomment-174539020 Yes, your import is correct, I've done a mistake, the collections4 (new collection version) isn't used by ACS, sry for 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: Refactor system VM default network creati...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1360#issuecomment-174571489 There isn't a test for this class if you want a suggestion, create a new folder named test in cloud-controller-secondary-storage project and create a new package in this folder named org.apache.cloudstack.secondarystorage with a class named SecondaryStorageManagerTest.java I think that is the correct pattern. I hope this helps. --- 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: CLOUDSTACK-8931: Fail to deploy VM instan...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/907#discussion_r51360120
--- Diff: server/src/com/cloud/network/IpAddressManagerImpl.java ---
@@ -680,10 +681,14 @@ public IPAddressVO doInTransaction(TransactionStatus
status) throws Insufficient
// If owner has dedicated Public IP ranges, fetch IP from the
dedicated range
// Otherwise fetch IP from the system pool
-List maps =
_accountVlanMapDao.listAccountVlanMapsByAccount(owner.getId());
-for (AccountVlanMapVO map : maps) {
-if (vlanDbIds == null || vlanDbIds.contains(map.getVlanDbId()))
-dedicatedVlanDbIds.add(map.getVlanDbId());
+Network network = _networksDao.findById(guestNetworkId);
+//Checking if network is null in the case of system VM's. At the
time of allocation of IP address to systemVm, no network is present.
+if(network == null || !(network.getGuestType() == GuestType.Shared
&& zone.getNetworkType() == NetworkType.Advanced)) {
--- End diff --
Hi, kansal.
Could you extract this 'if' content to a method with a little test case and
a Javadoc?
You had explained this 'if' pretty good in the comment above, but I think
that is a little confuse to understand yet, I know that you can explain this if
better and use a Javadoc to do it.
Ty.
---
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: CLOUDSTACK-8609. [VMware] VM is not acces...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/556#discussion_r51372149
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -1765,7 +1767,18 @@ protected StartAnswer execute(StartCommand cmd) {
// Since VM was successfully powered-on, if there was an
existing VM in a different cluster that was unregistered, delete all the files
associated with it.
if (existingVmName != null && existingVmFileLayout != null) {
-deleteUnregisteredVmFiles(existingVmFileLayout, dcMo,
true);
+List skipDatastores = new ArrayList();
+List vmDatastoreNames = new ArrayList();
+for (DatastoreMO vmDatastore : vmMo.getAllDatastores()) {
+vmDatastoreNames.add(vmDatastore.getName());
+}
+// Don't delete files that are in a datastore that is
being used by the new VM as well (zone-wide datastore).
+for (DatastoreMO existingDatastore : existingDatastores) {
+if
(vmDatastoreNames.contains(existingDatastore.getName())) {
+skipDatastores.add(existingDatastore.getName());
+}
+}
+deleteUnregisteredVmFiles(existingVmFileLayout, dcMo,
true, skipDatastores);
}
--- End diff --
Hi, likitha.
Could you extract the code between lines 1770 - 1775 and 1776 - 1780 to
methods with a little test case and Javadoc explaining what the methods do, the
params that they use and what they return?
Another thing, how about you replace the logic in 'if's at lines 2272, 2283
and 2291 for a method with a little test case and a Javadoc, that receives 2
params, (List skipDatastores, String name ) and returns skipDatastores
== null || !skipDatastores.contains(name).
Ty.
---
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: CLOUDSTACK-8894: Restrict vGPU enabled VM...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/868#discussion_r52022212
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -1535,6 +1535,19 @@ private boolean upgradeRunningVirtualMachine(Long
vmId, Long newServiceOfferingI
+ ",memory=," + currentMemory + ")");
}
+_offeringDao.loadDetails(currentServiceOffering);
+_offeringDao.loadDetails(newServiceOffering);
+
+Map currentDetails =
currentServiceOffering.getDetails();
+Map newDetails = newServiceOffering.getDetails();
+String currentVgpuType = currentDetails.get("vgpuType");
+String newVgpuType = newDetails.get("vgpuType");
+if(currentVgpuType != null) {
+if(newVgpuType == null ||
!newVgpuType.equalsIgnoreCase(currentVgpuType)) {
+throw new InvalidParameterValueException("Dynamic scaling
of vGPU type is not supported. VM has vGPU Type: " + currentVgpuType);
+}
+}
+
// Check resource limits
--- End diff --
Hi @anshul1886,
Could you extract the codes between lines 1541 - 1549 to a method with an
auto described name, do some test cases to this new method and create a Javadoc
explaining what the method do, what params it receive, the exceptions it throws
and what can cause the exception?
Ty.
---
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: CLOUDSTACK-8856 Primary Storage Used(type...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/865#discussion_r52029971
--- Diff: server/src/com/cloud/server/ManagementServerImpl.java ---
@@ -2488,6 +2479,23 @@ public int compare(final SummedCapacity arg0, final
SummedCapacity arg1) {
final SummedCapacity summedCapacity = new
SummedCapacity(capacity.getUsedCapacity(), capacity.getTotalCapacity(),
capacity.getUsedPercentage(),
capacity.getCapacityType(),
capacity.getDataCenterId(), capacity.getPodId(), capacity.getClusterId());
list.add(summedCapacity);
+}
+} else {
+List dcList = _dcDao.listEnabledZones();
+for (DataCenterVO dc : dcList) {
+List capacities=new
ArrayList();
+
capacities.add(_storageMgr.getSecondaryStorageUsedStats(null, dc.getId()));
+
capacities.add(_storageMgr.getStoragePoolUsedStats(null, null, null,
dc.getId()));
+for (CapacityVO capacity : capacities) {
+if (capacity.getTotalCapacity() != 0) {
+
capacity.setUsedPercentage((float)capacity.getUsedCapacity() /
capacity.getTotalCapacity());
+} else {
+capacity.setUsedPercentage(0);
+}
+SummedCapacity summedCapacity = new
SummedCapacity(capacity.getUsedCapacity(), capacity.getTotalCapacity(),
capacity.getUsedPercentage(),
+capacity.getCapacityType(),
capacity.getDataCenterId(), capacity.getPodId(), capacity.getClusterId());
+list.add(summedCapacity);
+}
}// End of for
--- End diff --
Hi @bvbharatk.
Could you extract lines 2489 - 2498 to a method like
"configureUsageCapacities" and a test case to this new method?
I believe you can also extract lines 2486 -2488 to a method like
"createCapacities" with test cases too.
Another suggestion for you, I know that you don't touch in this part of the
code, but how about you change the name of the variable declared at line 2464
to listOfSummedCapacity, the actual name "list" is a bit shallow.
If you find useful, a Java doc describing each methods task would be a plus.
Ty.
---
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: CLOUDSTACK-8829 : Consecutive cold migrat...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/797#discussion_r53579417
--- Diff:
engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java ---
@@ -1776,19 +1773,26 @@ private void orchestrateStorageMigration(final
String vmUuid, final StoragePool
// If VM was cold migrated between clusters belonging to
two different VMware DCs,
// unregister the VM from the source host and cleanup the
associated VM files.
if (vm.getHypervisorType().equals(HypervisorType.VMware)) {
+Long srcClusterId = null;
+Long srcHostId = vm.getHostId() != null ?
vm.getHostId() : vm.getLastHostId();
+if (srcHostId != null) {
+HostVO srcHost = _hostDao.findById(srcHostId);
+srcClusterId = srcHost.getClusterId();
+}
+
final Long destClusterId = destPool.getClusterId();
if (srcClusterId != null && destClusterId != null && !
srcClusterId.equals(destClusterId)) {
final String srcDcName =
_clusterDetailsDao.getVmwareDcName(srcClusterId);
final String destDcName =
_clusterDetailsDao.getVmwareDcName(destClusterId);
if (srcDcName != null && destDcName != null &&
!srcDcName.equals(destDcName)) {
s_logger.debug("Since VM's storage was
successfully migrated across VMware Datacenters, unregistering VM: " +
vm.getInstanceName() +
-" from source host: " +
srcHost.getId());
+" from source host: " + srcHostId);
final UnregisterVMCommand uvc = new
UnregisterVMCommand(vm.getInstanceName());
uvc.setCleanupVmFiles(true);
try {
-_agentMgr.send(srcHost.getId(), uvc);
+_agentMgr.send(srcHostId, uvc);
} catch (final Exception e) {
-throw new CloudRuntimeException("Failed to
unregister VM: " + vm.getInstanceName() + " from source host: " +
srcHost.getId() +
+throw new CloudRuntimeException("Failed to
unregister VM: " + vm.getInstanceName() + " from source host: " + srcHostId +
--- End diff --
hi @maneesha-p
How about you replace the catch Exception at line 1794 to (
AgentUnavailableException | OperationTimedoutException )?
---
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: CLOUDSTACK-9182: Some running VMs turned ...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1252#discussion_r53579947
--- Diff:
engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java ---
@@ -1472,6 +1473,12 @@ private void advanceStop(final VMInstanceVO vm,
final boolean cleanUpEvenIfUnabl
_workDao.update(work.getId(), work);
}
return;
+} else {
+HostVO host = _hostDao.findById(hostId);
+if (!cleanUpEvenIfUnableToStop && vm.getState() ==
State.Running && host.getResourceState() ==
ResourceState.PrepareForMaintenance) {
+s_logger.debug("Host in PrepareForMaintenance state -
Failed to stop VM with id: " + vm.getId());
--- End diff --
hello @sureshanaparti.
How about you extract the content in the 'if' at line 1478 to an
"isHostPreparingForMaintenance" method or something like that? it will explain
better the if content, and you can do a test case to this method too
Ty.
---
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: Create test cases to getPatchFilePath met...
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/944 Create test cases to getPatchFilePath method and class names changed In this commit we created tests cases for the respective classes in package âcom.cloud.hypervisor.xenserver.resourceâ. We added test cases to check the implementation of âcom.cloud.hypervisor.xenserver.resource.CitrixResourceBase.getPatchFilesâ. Therefore, we test in a more comprehensive way the tests that already exist to check the code of âcom.cloud.hypervisor.xenserver.resource.CitrixResourceBase.getPatchFilePathâ. We added a new abstract class, called com.cloud.hypervisor.xenserver.resource.CitrixResourceBaseTest.java This class has two tests methods: * com.cloud.hypervisor.xenserver.resource.CitrixResourceBaseTest.testGetPathFilesExeption(CitrixResourceBase), this method tests if the getPatchFilePath() method throws the com.cloud.utils.exception.CloudRuntimeException.CloudRuntimeException when the com.cloud.utils.script.Script.findScript(String, String) return a null value; * com.cloud.hypervisor.xenserver.resource.CitrixResourceBaseTest.testGetPathFilesListReturned(CitrixResourceBase), this method tests the correct return value from getPatchFilePath() method, basically, verify if the returned list contain the file with the same absolute path that was retrieved from the findScript method. We also changed the name of those test classes, the change was basically remove the âPathâ word from the name of classes. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack lrg-cs-hackday-012 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/944.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 #944 commit b9e508c612dc361a21ccde85e31b251b16fcdff4 Author: pedro-martins Date: 2015-10-17T20:33:41Z created tests cases for the respective classes in package âcom.cloud.hypervisor.xenserver.resourceâ. also changed the name of those test classes, the change was basically remove the âPathâ word from the name of classes. commit 8ba4c672b564ee16d1c1cbb42979a0c456c6bbc8 Author: pedro-martins Date: 2015-10-17T20:36:40Z change the private method com.cloud.hypervisor.xenserver.resource.CitrixResourceBase.getPatchFiles to protected added new class in test cases --- 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: Create test cases to getPatchFilePath met...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/944#issuecomment-158667466 merged with master top version, someone can check this PR? =) thx guys. --- 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: Create test cases to getPatchFilePath met...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/944#issuecomment-158921322 Thanks for your review Daan. The purpose of this test is to check if an exception is thrown. The point is that, we have several class that we need to run the very same test; therefore, the method âCitrixResourceBaseTest. testGetPathFilesExeptionâ is used by methods such as âcom.cloud.hypervisor.xenserver.resource.XcpOssResourceTest.testGetFiles()â. Those methods (that are our actual test case methods) are expecting an exception. That is why that method does not have an assertion, it is expected an exception there. --- 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: Create test cases to getPatchFilePath met...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/944#issuecomment-158929670 Squash done. Thx guys --- 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: Removed the PlannerBase class because it ...
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/1108 Removed the PlannerBase class because it is does not bring contribution to ACS' code. Removed the PlannerBase class because it is does not bring contribution to ACS' code. We changed com.cloud.deploy.FirstFitPlanner, now it doesnât extends the PlannerBase and implements the com.cloud.deploy.DeploymentPlanner. We also removed the method com.cloud.deploy.DeploymentPlanner.check(VirtualMachineProfile, DeploymentPlan, DeployDestination, ExcludeList) that was not used anywhere. Additionally, we removed the â_â from FirstFitPlanner's attributes name, in order to have them in a more standard way. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack master-lrg-cs-hackday-004 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1108.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 #1108 commit 9eaaf439065a13477a3c65ed8f879f3b784f1dc7 Author: pedro-martins Date: 2015-11-23T14:15:05Z Removed the empty class 'com.cloud.deploy.PlannerBase' --- 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: Changed variable s_logger to non-static a...
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/714 Changed variable s_logger to non-static and fixed its name in âcom.cloud.utils.component.ComponentLifecycleBaseâ and its subclasses 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. 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/714.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 #714 commit 631dccada87c131f9b2106d9942d07cb6a61db79 Author: pedro-martins Date: 2015-08-18T14:54:20Z Changed 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) --- 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: Changed variable s_logger to non-static a...
Github user pedro-martins closed the pull request at: https://github.com/apache/cloudstack/pull/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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Changed variable s_logger to non-static a...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/714#issuecomment-132654938 @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: protected Logger logger = Logger.getLogger(getClass()); This way, every single subclass of ComponentLifecycleBase, when instantiated would automatically have a logger instance for its proper class ready to be used. --- 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: Changed variable s_logger to non-static a...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/714#issuecomment-132698414 Got your comments, sadly we have not analyzed which classes are singleton or not. We have opened a jira ticket to that: https://issues.apache.org/jira/browse/CLOUDSTACK-8750 If someone need a hand, just shoot an email. --- 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: Changed variable s_logger to non-static a...
GitHub user pedro-martins reopened a pull request: https://github.com/apache/cloudstack/pull/714 Changed variable s_logger to non-static and fixed its name in âcom.cloud.utils.component.ComponentLifecycleBaseâ and its subclasses 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. 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/714.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 #714 commit ddcd1ed3e53dd3fff6e557e4a4b13eb2f03e62df Author: pedro-martins Date: 2015-08-19T14:02:37Z Changed variable s_logger to non-static and fixed its name in âcom.cloud.utils.component.ComponentLifecycleBaseâ and its subclasses 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) test test2 --- 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: Removed the empty class 'com.cloud.deploy...
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/856 Removed the empty class 'com.cloud.deploy.PlannerBase' Removed the PlannerBase class because it is does not bring contribution to ACS' code. We changed com.cloud.deploy.FirstFitPlanner, now it doesnât extends the PlannerBase and implements the com.cloud.deploy.DeploymentPlanner. We also removed the method com.cloud.deploy.DeploymentPlanner.check(VirtualMachineProfile, DeploymentPlan, DeployDestination, ExcludeList) that was not used anywhere. Additionally, we removed the â_â from FirstFitPlanner's attributes name, in order to have them in a more standard way. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack master-lrg-cs-hackday-004 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/856.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 #856 commit d5a22526f1fe895d641d13ff0dea8505e2d949a2 Author: pedro-martins Date: 2015-09-19T20:10:21Z Removed class com.cloud.deploy.PlannerBase removed method com.cloud.deploy.DeploymentPlanner.check(VirtualMachineProfile, DeploymentPlan, DeployDestination, ExcludeList) modified the com.cloud.deploy.FirstFitPlanner for extends AdapterBase and implements DeploymentPlanner removed "_" from variables name --- 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: Removed the empty class 'com.cloud.deploy...
Github user pedro-martins closed the pull request at: https://github.com/apache/cloudstack/pull/856 --- 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: Removed the empty class 'com.cloud.deploy...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/856#issuecomment-142042882 Reopening PR that was closed by mistake --- 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: Removed the empty class 'com.cloud.deploy...
GitHub user pedro-martins reopened a pull request: https://github.com/apache/cloudstack/pull/856 Removed the empty class 'com.cloud.deploy.PlannerBase' Removed the PlannerBase class because it is does not bring contribution to ACS' code. We changed com.cloud.deploy.FirstFitPlanner, now it doesnât extends the PlannerBase and implements the com.cloud.deploy.DeploymentPlanner. We also removed the method com.cloud.deploy.DeploymentPlanner.check(VirtualMachineProfile, DeploymentPlan, DeployDestination, ExcludeList) that was not used anywhere. Additionally, we removed the â_â from FirstFitPlanner's attributes name, in order to have them in a more standard way. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack master-lrg-cs-hackday-004 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/856.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 #856 --- 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: CLOUDSTACK-9298: Improve performance of r...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1425#issuecomment-190791313 @nvazquez, nice changes. So, I guess that a test cases for these new methods will be important to detect future changes in these methods behavior. Another thing, is this new constructor really necessary? do you use this constructor in any part of the code? or do you just created this constructor to improve future codes calling only this constructor instead of lots of setters as you said? In my opinion, a constructor with many params is a bit ugly, I prefer to use no params in the constructor and use some factory pattern to create this object (lots of setters called inside a factory class), this is just my opinion. Ty. --- 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: [CLOUDSTACK-8973] Fix create template fro...
Github user pedro-martins commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1424#discussion_r54595010 --- Diff: server/src/com/cloud/template/TemplateManagerImpl.java --- @@ -275,6 +277,8 @@ private StorageCacheManager cacheMgr; @Inject private EndPointSelector selector; +@Inject +private ImageStoreDao _imgStoreDao; --- End diff -- hi @syed, could you remove the "_" character from the variable name? the underscore is usually used in c++ programs to name private variables but it is not a convention in Java. Ty. --- 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: CLOUDSTACK-9298: Improve performance of r...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1425#issuecomment-190874260 @nvazquez I mean unit tests for each new method. I think that use the Builder to get an instance with all variables initialized is a good way. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9298: Improve performance of r...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1425#discussion_r54797480
--- Diff: server/test/com/cloud/api/query/dao/TemplateJoinDaoImplTest.java
---
@@ -0,0 +1,56 @@
+package com.cloud.api.query.dao;
+
+import junit.framework.TestCase;
+
+import org.apache.cloudstack.api.response.ResourceTagResponse;
+import org.apache.cloudstack.api.response.TemplateResponse;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import com.cloud.api.ApiDBUtils;
+import com.cloud.api.query.vo.TemplateJoinVO;
+import com.cloud.user.AccountService;
+
+@RunWith(PowerMockRunner.class)
+@PrepareForTest(ApiDBUtils.class)
+public class TemplateJoinDaoImplTest extends TestCase {
+
+@Mock(name="_configDao")
+private ConfigurationDao _configDao;
+
+@Mock(name="_accountService")
+private AccountService _accountService;
+
+@InjectMocks
+private TemplateJoinDaoImpl _templateJoinDaoImpl;
+
+private TemplateJoinVO template = new TemplateJoinVO();
+private TemplateResponse templateResponse = new TemplateResponse();
+
+private final static long TAG_ID = 1l;
+private final static String TAG_UUID = "---";
+
+@Before
+public void setup() {
+MockitoAnnotations.initMocks(this);
+template.setTagId(TAG_ID);
+template.setTagUuid(TAG_UUID);
+PowerMockito.spy(ApiDBUtils.class);
+PowerMockito.stub(PowerMockito.method(ApiDBUtils.class,
"newResourceTagResponse")).toReturn(new ResourceTagResponse());
+}
+
+@Test
+public void testUpdateTemplateTagInfo(){
+assertEquals(0, templateResponse.getTags().size());
+_templateJoinDaoImpl.updateTemplateTagInformation(template,
templateResponse);
+assertEquals(1, templateResponse.getTags().size());
+}
--- End diff --
Hi, @nvazquez.
I see a little mistake in you test, you are testing if the method has
inserted a tag or not, I think that this test needs to verify if the template
inserted in the templateResponse is the same that you has passed in the first
param of the method.
You can do it checking if each variable in both templates are equals.
The same problem in the other tests cases.
Ty.
---
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: CLOUDSTACK-9298: Improve performance of r...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1425#issuecomment-192286165 @nvazquez. Could you squash all of yours commits into one? It's hard to compare the original code with your current modifications. If you do not know how to squash here goes a [link](https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/) with a little tutorial --- 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: Change variable "ROOK_DISK_CONTROLLER" to...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1434#issuecomment-194957493 Simple change in variable name, 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: removed unused HypervDummyResourceBase cl...
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/1437 removed unused HypervDummyResourceBase class Was removed the class com.cloud.hypervisor.hyperv.resource.HypervDummyResourceBase - This class do not have the annotation @Component and there are no beans that instantiate this class in XML files, then this class isn't in the spring lifecycle; - The constructor of this class isn't called in the entire ACS code; - There is no other class that extends this class. Well, I think that this class isn't used in the ACS code, but if I'm wrong, please be free to comment. Ty. You can merge this pull request into a Git repository by running: $ git pull https://github.com/pedro-martins/cloudstack removed-unused-HypervDummyResourceBase-class Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1437.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 #1437 commit e1a75de9bc46776390d05aa1e996861a5a1e053a Author: pedro-martins Date: 2016-03-10T17:46:33Z removed unused com.cloud.hypervisor.hyperv.resource.HypervDummyResourceBase class --- 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: removed unused HypervDummyResourceBase cl...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1437#issuecomment-195361064 @rafaelweingartner ty for your review. Changed the log message. Ty. --- 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: removed unused HypervDummyResourceBase cl...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1437#issuecomment-195364975 Hi @eriweb, thanks for reviewing this PR. I did a maven install in the ACS project and it built and ran all tests cases with no errors. But I didn't the integration test. Ty. --- 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: Removed unused parameters and variable fr...
GitHub user pedro-martins opened a pull request: https://github.com/apache/cloudstack/pull/1447 Removed unused parameters and variable from NetworkHelper hierarchy - Was removed the unused params User & Account in the com.cloud.network.router.NetworkHelper.startVirtualRouter(DomainRouterVO, User, Account, Map) and the param Long in the com.cloud.network.router.NetworkHelper.destroyRouter(long, Account, Long) - Also, was removed the unused params User & Account from com.cloud.network.router.NetworkHelperImpl.start(DomainRouterVO, User, Account, Map, DeploymentPlan) You can merge this pull request into a Git repository by running: $ git pull https://github.com/pedro-martins/cloudstack lrg-cs-hackday-032 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1447.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 #1447 commit 35bfb918828d3f69a2ae549cedce40177e9bd2aa Author: pedro-martins Date: 2016-03-19T22:42:35Z remove unused parameters and variable from NetworkHelper hierarchy --- 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: Removed unused parameters and variable fr...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1447#issuecomment-199337285 Hi @DaanHoogland. I did a maven install (this executes the unit tests). Also, the PR has passed in the Jenkins and the CI tests. However, I did not execute functional tests (integration tests). The analysis was done manually; basically, I saw that a method was not using some of its parameters. After that, I checked that this method is an implementation of an interface's method, then I checked all classes that implement that interface (just one), then I analyzed all of the methods in that interface to detect if any other had the same problem with unused parameters and fixed it. Ty. --- 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: CLOUDSTACK-9322: Support for Internal LB ...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1452#issuecomment-202163598 Hi @prashanthvarma. How about to use String.format() to create the strings in the loggers? The use of String format will turn the strings in the logs more legible. Also, at line 344, could you use Collection.isEmpty(internalLbVms) instead of "internalLbVms == null || internalLbVms.isEmpty()" ? Ty. --- 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: CLOUDSTACK-9319: Use timeout when applyin...
Github user pedro-martins commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1451#discussion_r57537243
--- Diff:
core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java ---
@@ -180,7 +179,7 @@ private Answer applyConfig(NetworkElementCommand cmd,
List cfg) {
boolean finalResult = false;
for (ConfigItem configItem : cfg) {
long startTimestamp = System.currentTimeMillis();
-ExecutionResult result =
applyConfigToVR(cmd.getRouterAccessIp(), configItem);
+ExecutionResult result =
applyConfigToVR(cmd.getRouterAccessIp(), configItem,
VRScripts.DEFAULT_EXECUTEINVR_TIMEOUT);
if (s_logger.isDebugEnabled()) {
long elapsed = System.currentTimeMillis() - startTimestamp;
s_logger.debug("Processing " + configItem + " took " +
elapsed + "ms");
--- End diff --
Hi @insom .
I know that this isn't your code but could you use String.format to create
the string that is used by logger?
It turns the code more readable than using multiple strings concatenation.
Ty.
---
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: CLOUDSTACK-9322: Support for Internal LB ...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1452#issuecomment-203581569 @nlivens great! And sorry for the delay =) I saw that you used the String.format as I suggested, but and about the using of CollectionUtils.isEmpty() in the class org.apache.cloudstack.network.element.InternalLoadBalancerElement.java at line 344? Did you don't like the suggestion or you just has forgotten to change? 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9298: Improve performance of r...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1425#issuecomment-203654592 @nvazquez, Nice changes, great job refactoring your implementation. I found no more troubles in this PR, then LGTM. Ty. --- 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: Removed unused methods from EventBus inte...
Github user pedro-martins closed the pull request at: https://github.com/apache/cloudstack/pull/1260 --- 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: Remove unused params from NetworkHelperIm...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1484#issuecomment-208086436 Hi @GabrielBrascher . I think this params are removed in this PR https://github.com/apache/cloudstack/pull/1447 =) Ty. --- 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: Remove unused params from NetworkHelperIm...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1484#issuecomment-208087293 @rafaelweingartner I will open a Jira, the way that I found the problem was described in an answer to Daan Hoogland in the same PR. Ty --- 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: Remove unused params from NetworkHelperIm...
Github user pedro-martins commented on the pull request: https://github.com/apache/cloudstack/pull/1484#issuecomment-208088632 @rafaelweingartner Done. the Jira is CLOUDSTACK-9343 if you can take a look. Ty --- 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. ---
