[GitHub] cloudstack pull request: Undetected bug correct and refactor of th...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1499#issuecomment-221921405 @swill Sure can do, sorry for the delay, I haven't had much time lately but that will be done soon. --- 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 alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1280#issuecomment-217842674 as @pedro-martins stated, it seems to be fitting that this method is extracted to a class to be documented/tested. The code looks good but if the exception is to be launched I see no room for backward compatibility. If that's an issue, this needs to be rethinked as the raised exception would have to be treated and at that point we would have to think if raising the exception was really necessary. Overall the code looks good, simple and justified. --- 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-8958: release dedicated ip ran...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1357#issuecomment-217840055 Is there a test already to check if the ip ranges release method is called? If there is none, I think it should be added --- 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-9365 : updateVirtualMachine wi...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1523#issuecomment-216192751 That seems fair... But I think you could make 3 unit tests instead of 1 so each one would have only 1 assert and only one behavior each. Other than that it all looks fine. As Daan said, the code seems sane and the tests prove it's functionality. --- 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-9358: StringIndexOutOfBoundsEx...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1503#discussion_r61723859
--- Diff: server/src/com/cloud/api/ApiServer.java ---
@@ -264,10 +266,11 @@ public void handleAsyncJobPublishEvent(String
subject, String senderAddress, Obj
String info = job.getCmdInfo();
String cmdEventType = "unknown";
if (info != null) {
-String marker = "\"cmdEventType\"";
-int begin = info.indexOf(marker);
-if (begin >= 0) {
-cmdEventType = info.substring(begin + marker.length() + 2,
info.indexOf(",", begin) - 1);
+Type type = new TypeToken<Map<String, Object>>(){}.getType();
+Map<String, Object> cmdInfo =
ApiGsonHelper.getBuilder().create().fromJson(info, type);
--- End diff --
is this Object necessary as the mapped type? Couldn't it be String
directly? If you are using the api solely for collecting the cmdEventType, you
could build the objects as String already and the typecheck on line 272 would
be unnecessary.
---
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-9365 : updateVirtualMachine wi...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1523#issuecomment-216070253 @cristofolini that would not be that simple, there is userDataApplied variable which is extern to the loop. It would be better to turn lines 2496-2515 intto a Nic's class method which returns a boolean in this case. @nilves you should do a simple testcase to this problem, that would minimize the ammount of scripts needed to test such features. --- 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-9366: Capacity of one zone-wid...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1516#discussion_r60852850
--- Diff: engine/schema/src/com/cloud/capacity/dao/CapacityDaoImpl.java ---
@@ -962,35 +962,59 @@ public boolean removeBy(Short capacityType, Long
zoneId, Long podId, Long cluste
}
@Override
-public void updateCapacityState(Long dcId, Long podId, Long clusterId,
Long hostId, String capacityState) {
+public void updateCapacityState(Long dcId, Long podId, Long clusterId,
Long hostId, String capacityState, short[] capacityType) {
TransactionLegacy txn = TransactionLegacy.currentTxn();
StringBuilder sql = new StringBuilder(UPDATE_CAPACITY_STATE);
List resourceIdList = new ArrayList();
+StringBuilder where = new StringBuilder();
if (dcId != null) {
-sql.append(" data_center_id = ?");
+where.append(" data_center_id = ? ");
resourceIdList.add(dcId);
}
if (podId != null) {
-sql.append(" pod_id = ?");
+where.append((where.length() > 0) ? " and pod_id = ? " : "
pod_id = ? ");
resourceIdList.add(podId);
}
if (clusterId != null) {
-sql.append(" cluster_id = ?");
+where.append((where.length() > 0) ? " and cluster_id = ? " : "
cluster_id = ? ");
resourceIdList.add(clusterId);
}
if (hostId != null) {
-sql.append(" host_id = ?");
+where.append((where.length() > 0) ? " and host_id = ? " : "
host_id = ? ");
resourceIdList.add(hostId);
}
+if (capacityType != null && capacityType.length > 0) {
+where.append((where.length() > 0) ? " and capacity_type in " :
" capacity_type in ");
+
+StringBuilder builder = new StringBuilder();
+for( int i = 0 ; i < capacityType.length; i++ ) {
+if(i==0){
+builder.append(" (? ");
--- End diff --
You can extract the if(i==0) code from the for and write it before the
loop. That way you don`t need to check anything inside the loop
---
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-9350: KVM-HA- Fix CheckOnHost ...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1496#discussion_r60004052
--- Diff: server/src/com/cloud/ha/HighAvailabilityManagerImpl.java ---
@@ -264,6 +265,11 @@ public void scheduleRestartForVmsOnHost(final HostVO
host, boolean investigate)
"Host [" + hostDesc + "] is down." + ((sb != null) ?
sb.toString() : ""));
for (VMInstanceVO vm : reorderedVMList) {
+ServiceOfferingVO vmOffering =
_serviceOfferingDao.findById(vm.getServiceOfferingId());
--- End diff --
I would recommend putting this changed inside the if scope in line 273. The
additions are related to debug and if the debug is disabled, you can skip it.
---
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: Fixes regarding VOLUME_DELETE events resu...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1491#discussion_r60003773
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -677,6 +679,17 @@ public boolean deleteAccount(AccountVO account, long
callerUserId, Account calle
return cleanupAccount(account, callerUserId, caller);
}
+protected List getExpungedInstanceRootVolume(long
instanceId) {
+SearchBuilder sb = _volumeDao.createSearchBuilder();
+sb.and("instanceId", sb.entity().getInstanceId(),
SearchCriteria.Op.EQ);
+sb.and("vType", sb.entity().getVolumeType(), SearchCriteria.Op.EQ);
+sb.done();
+SearchCriteria c = sb.create();
+c.setParameters("instanceId", instanceId);
+c.setParameters("vType", Volume.Type.ROOT);
+return _volumeDao.customSearchIncludingRemoved(c, null);
+}
+
protected boolean cleanupAccount(AccountVO account, long callerUserId,
Account caller) {
--- End diff --
I think it is missing a simple unit tests as a test if the method
expungedInstaceRootVolume is called when the cleanupAccount is invoked and
there is a vm in destroyed state. That and the guarantee that the method
getExpungedInstanceRootVolume works, added to the tests you made would complete
it.
---
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: Undetected bug correct and refactor of th...
GitHub user alexandrelimassantana opened a pull request: https://github.com/apache/cloudstack/pull/1499 Undetected bug correct and refactor of the class NfsSecondaryStorageResource Im in process of rewriting the unit test for this class' mount method. Before I can do that, the class needed some refactoring, not just because it was fuzzy but because it was impossible to use mocks the way it was coded. Most of this PR is about transforming big chunks of code into documented methods with two exceptions: 1) I inverted the logic when checking for existing mounts within the same root location. Previously one would first create a directory if it didn't exist and then test if there was already mount there. Now it is first testing if there is a mount, if not, it creates the needed directory. 2) A bug was found in the unmount method. The unmount wasn't issuing the command "umount" to unmount a resource but instead was trying the "mount" command again, resulting in mounted resources never being unmounted. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack lrg-cs-hackday-046 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1499.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 #1499 commit d85810d122ec9f5c697ad77f6a5422e50ac718da Author: Alexandre Limas Santana <alexandre.limas.sant...@gmail.com> Date: 2016-04-16T20:49:49Z Refactor of the class NfsSecondaryStorageResource withing the domain of the mount process. A bug was found and corrected in the function unmount --- 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-9287 - Fix unique mac address ...
Github user alexandrelimassantana commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1483#discussion_r59121072 --- Diff: server/src/com/cloud/network/element/VpcVirtualRouterElement.java --- @@ -466,7 +466,7 @@ public boolean deletePrivateGateway(final PrivateGateway gateway) throws Concurr } } -return result > 0 ? true : false; +return result == routers.size() ? true : false; } --- End diff -- This line is equivallent to ```Java return result == routers.size(); ``` please take the ternary operator away --- 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-9337]Enhance vcenter.py to cr...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1464#discussion_r58739597
--- Diff: tools/marvin/marvin/lib/vcenter.py ---
@@ -183,7 +192,157 @@ def get_clusters(self, dc, clus=None):
"""
pass
+def create_datacenter(self, dcname=None, service_instance=None,
folder=None):
+"""
+Creates a new datacenter with the given name.
+Any % (percent) character used in this name parameter must be
escaped,
+unless it is used to start an escape sequence. Clients may also
escape
+any other characters in this name parameter.
+
+An entity name must be a non-empty string of
+less than 80 characters. The slash (/), backslash (\) and percent
(%)
+will be escaped using the URL syntax. For example, %2F
+
+This can raise the following exceptions:
+vim.fault.DuplicateName
+vim.fault.InvalidName
+vmodl.fault.NotSupported
+vmodl.fault.RuntimeFault
+ValueError raised if the name len is > 79
+https://github.com/vmware/pyvmomi/blob/master/docs/vim/Folder.rst
+
+Required Privileges
+Datacenter.Create
+
+:param folder: Folder object to create DC in. If None it will
default to
+ rootFolder
+:param dcname: Name for the new datacenter.
+:param service_instance: ServiceInstance connection to a given
vCenter
+:return:
+"""
+if len(dcname) > 79:
+raise ValueError("The name of the datacenter must be under "
+ "80 characters.")
+
+if folder is None:
+folder = self.service_instance.content.rootFolder
+
+if folder is not None and isinstance(folder, vim.Folder):
+dc_moref = folder.CreateDatacenter(name=dcname)
--- End diff --
got it. The code looks good, those were the only doubts I had. I would just
use a Semaphore in line 317, but sleep an pool is not that bad also.
The code seems fine, but tests are needed.
---
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-9337]Enhance vcenter.py to cr...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1464#discussion_r58633962
--- Diff: tools/marvin/marvin/lib/vcenter.py ---
@@ -183,7 +192,157 @@ def get_clusters(self, dc, clus=None):
"""
pass
+def create_datacenter(self, dcname=None, service_instance=None,
folder=None):
+"""
+Creates a new datacenter with the given name.
+Any % (percent) character used in this name parameter must be
escaped,
+unless it is used to start an escape sequence. Clients may also
escape
+any other characters in this name parameter.
+
+An entity name must be a non-empty string of
+less than 80 characters. The slash (/), backslash (\) and percent
(%)
+will be escaped using the URL syntax. For example, %2F
+
+This can raise the following exceptions:
+vim.fault.DuplicateName
+vim.fault.InvalidName
+vmodl.fault.NotSupported
+vmodl.fault.RuntimeFault
+ValueError raised if the name len is > 79
+https://github.com/vmware/pyvmomi/blob/master/docs/vim/Folder.rst
+
+Required Privileges
+Datacenter.Create
+
+:param folder: Folder object to create DC in. If None it will
default to
+ rootFolder
+:param dcname: Name for the new datacenter.
+:param service_instance: ServiceInstance connection to a given
vCenter
+:return:
+"""
+if len(dcname) > 79:
+raise ValueError("The name of the datacenter must be under "
+ "80 characters.")
+
+if folder is None:
+folder = self.service_instance.content.rootFolder
+
+if folder is not None and isinstance(folder, vim.Folder):
+dc_moref = folder.CreateDatacenter(name=dcname)
--- End diff --
Hello @sanju1010
Is it necessary to double check if folder is not None here? Also you
mentioned some restrictions for the dcname, shouldnt they also raise and
ValueError?
---
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-9328]: Fix vlan issues from t...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1455#issuecomment-202874549 Does the _cleanup_resources(cls.api_client, cls._cleanup)_ cleans the db connection as well? I see that it is only 1 test as of now, but if there would be another, the setUp should also work correctly? --- 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-9323: Fix cancel host maintena...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r57714466
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2112,11 +2112,13 @@ private boolean doCancelMaintenance(final long
hostId) {
/* TODO: move to listener */
_haMgr.cancelScheduledMigrations(host);
+
+boolean vms_migrating = false;
final List vms = _haMgr.findTakenMigrationWork();
for (final VMInstanceVO vm : vms) {
if (vm != null && vm.getHostId() != null && vm.getHostId() ==
hostId) {
s_logger.info("Unable to cancel migration because the vm
is being migrated: " + vm);
--- End diff --
Can you turn the if on line 2119 to a method call like _isVmMigrating(vm,
hostId)_? Or even _vm.isMigrating(hostId)_
I think that it will improve the readability of this segment you are
working. Also... is there a need to check all VMs ? Once you find one that is
migrating do you still need to keep checking if they are migrating? If there is
not a need, try changing the loop for a while, or issuing a break.
---
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 alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1451#issuecomment-202161988 @cristofolini there is no _timeout_ variable in the scope you commented. His change is valid because _VRScripts.DEFAULT_EXECUTEINVR_TIMEOUT_ is mapped to the previous default _timeout_ value. This is equivalent to the previous applyConfigToVR() call without passing the timout value. --- 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: Check the existence of 'forceencap' param...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1402#issuecomment-201681267 @remibergsma can't you asking false instead of "false" ? If the function accepts boolean values I think that it would be more efficient. Besides that, the addition is quite straightforward and I would be happy to hand out my 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: CLOUDSTACK-9319: Use timeout when applyin...
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1451#issuecomment-201610435
@insom with this changes there is still one place where the applyConfigToVR
is not provided with this timeout parameter (line 183, same class). This
function is actually called only in those 2 places. If it comes to a state
where you will always supply the timeout, remove the one which doesn't require
you to.
Also, if you plan to always supply the timeout, a change into lines 378 and
379 would provide more coersion:
```Java
if (timeout < 120) {
timeout = 120;
}
```
into:
```Java
if (timeout < VRScripts.DEFAULT_EXECUTEINVR_TIMEOUT) {
timeout = VRScripts.DEFAULT_EXECUTEINVR_TIMEOUT;
}
```
---
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: Fixed Profiler's unit tests bugs.
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1445#discussion_r56808020
--- Diff: utils/src/test/java/com/cloud/utils/TestProfiler.java ---
@@ -19,84 +19,79 @@
package com.cloud.utils;
-import org.apache.log4j.Logger;
import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
import com.cloud.utils.testcase.Log4jEnabledTestCase;
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({Profiler.class})
public class TestProfiler extends Log4jEnabledTestCase {
-protected final static Logger s_logger =
Logger.getLogger(TestProfiler.class);
-private static final long MILLIS_FACTOR = 1000l;
-private static final int MARGIN = 100;
-// Profiler uses System.nanoTime which is not reliable on the
millisecond
-// A sleep of 1000 milliseconds CAN be measured as 999 milliseconds
-// Therefore make the second larger, by 10% of the margin
-private static final long ONE_SECOND = 1000l + (MARGIN / 10);
-private static final double EXPONENT = 3d;
+private static final long SLEEP_TIME_NANO = 10L;
+private static Profiler pf;
+
+@Before
+public void setUp() {
+pf = new Profiler();
+PowerMockito.mockStatic(System.class);
+PowerMockito.when(System.nanoTime()).thenReturn(0L,
SLEEP_TIME_NANO);
--- End diff --
The statement on line 43 is saying that on the first call to nanoTime it
will return 0 and the second call will return 10^9 ( 1 second in nanoseconds ).
when start() calls nanoTime() first, it gets 0 and when stop() calls it
again, the result will be 10^9. That is how it simulates the passage of 1s
without actually making the thread sleep for 1 second.
The tests previously written about this class are not intended to test the
capability of the System class to capture time nor the Thread.sleep() method (
which was causing the test to fail as I stated because of the low priority the
JVM's proccess have on different OS ). As it is right now, it stills test the
same functionalities... Just without the sleep and System direct calls.
---
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-6928: fix issue disk I/O throt...
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1410#issuecomment-199090954
Hello @ustcweizhou
I am curious to know if this segment is really necessary:
```Java
disk = new DiskTO(volumeTO, vol.getDeviceId(), vol.getPath(),
vol.getVolumeType());
} else {
disk = new DiskTO(volTO, vol.getDeviceId(), vol.getPath(),
vol.getVolumeType());
{
```
volumeTO and volTO are the same variable.. just with a different cast
---
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-9297 - Reworked logic in Stora...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1441#discussion_r56768712
--- Diff:
engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java
---
@@ -440,41 +400,28 @@ private void markAsBackedUp(SnapshotObject
snapshotObj) {
@Override
public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation
op) {
-long volumeId = snapshot.getVolumeId();
-VolumeVO volumeVO = _volumeDao.findById(volumeId);
if (SnapshotOperation.REVERT.equals(op)) {
-if (volumeVO != null &&
ImageFormat.QCOW2.equals(volumeVO.getFormat()))
-return StrategyPriority.DEFAULT;
-else
-return StrategyPriority.CANT_HANDLE;
+return StrategyPriority.CANT_HANDLE;
}
--- End diff --
Good work on rewritting the logic to a slimmer state.
Wouldn't be it more readable if you turn lines 407-422 into a boolean
function which would return true if "supportsStorageSystemSnapshots". Snap this
chunk of code could prove useful on creating unit tests and documentation as
this segment clearly aims to check this condition solely.
---
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: Fixed Profiler's unit tests bugs.
GitHub user alexandrelimassantana opened a pull request: https://github.com/apache/cloudstack/pull/1445 Fixed Profiler's unit tests bugs. Problem: The TestProfiler class was using Java Thread methods to test the Profiler's functionality. That was causing the tests to fail sometimes since the JVM's thread priority could be low on some OS. Fix: Using PowerMockito to mock the System calls, the threads could be removed. This makes the tests considerably faster, OS independent and still guarantees the correct implementation of the Profiler class. The changes on the Profiler's class was only to shorten the class's line size by not assigning the return value to a variable returning it straight out. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack lrg-cs-hackday-025 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1445.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 #1445 commit 8d6370b21fd527dc0046c0a1d67dce35dbf15681 Author: Alexandre Limas Santana <alexandre.limas.sant...@gmail.com> Date: 2016-03-19T19:25:47Z Fixed Profiler's unit tests bugs. Problem: The TestProfiler class was using Java Thread methods to test the Profiler's functionality. That was causing the tests to fail sometimes since the JVM's thread priority could be low on some OS. Fix: Using PowerMockito to mock the System calls, the threads could be removed. This makes the tests considerably faster, OS independent and still guarantees the correct implementation of the Profiler class. The changes on the Profiler's class was only to shorten the class's line size by not assigning the return value to a variable returning it straight out. --- 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-9136: remove ssh keypairs alon...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1212#discussion_r55956795
--- Diff: server/src/com/cloud/server/ManagementServerImpl.java ---
@@ -3567,7 +3567,17 @@ public boolean deleteSSHKeyPair(final
DeleteSSHKeyPairCmd cmd) {
final Long domainId = cmd.getDomainId();
final Long projectId = cmd.getProjectId();
-final Account owner = _accountMgr.finalizeOwner(caller,
accountName, domainId, projectId);
+Account owner = null;
--- End diff --
Hello @ustcweizhou
I think this piece of code could be extracted from the deleteSSHKeyPair
method. It has gotten a little big from it's past form and further improvements
could be done outside of the main "deleteSSHKeyPair" flow if needed.
---
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-8857 listProjects doesn't retu...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/838#issuecomment-196137905 Hello @bvbharatk, I think a simple change like this could have a test case to test it, so we can avoid manual tests. --- 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 alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1252#discussion_r55169762
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -4468,6 +4472,16 @@ private boolean checkIfHostIsDedicated(HostVO host) {
}
}
+private void checkIfHostOfVMIsInPrepareForMaintenanceState(Long
hostId, Long vmId, String operation) {
--- End diff --
Hello @sureshanaparti
I think this method can use a Javadoc. Since it's supposed to check
something and doesn't return nothing, it might make someone have to open the
code to check what does it do.
---
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-9174: A deleted account result...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1254#discussion_r55169349
--- Diff:
framework/quota/src/org/apache/cloudstack/quota/QuotaManagerImpl.java ---
@@ -358,10 +358,11 @@ public QuotaUsageVO updateQuotaDiskUsage(UsageVO
usageRecord, final BigDecimal a
BigDecimal rawusage;
// get service offering details
ServiceOfferingVO serviceoffering =
_serviceOfferingDao.findServiceOffering(usageRecord.getVmInstanceId(),
usageRecord.getOfferingId());
+if (serviceoffering == null) return quotalist;
rawusage = new BigDecimal(usageRecord.getRawUsage());
QuotaTariffVO tariff =
_quotaTariffDao.findTariffPlanByUsageType(QuotaTypes.CPU_NUMBER,
usageRecord.getEndDate());
-if (tariff != null &&
tariff.getCurrencyValue().compareTo(BigDecimal.ZERO) != 0) {
+if (tariff != null &&
tariff.getCurrencyValue().compareTo(BigDecimal.ZERO) != 0 &&
serviceoffering.getCpu() != null) {
--- End diff --
Hello @agneya2001 ,
In the refactor you plan to make, you could also turn this piece of code
into a method, for reusability:
```Java
tariff != null && tariff.getCurrencyValue().compareTo(BigDecimal.ZERO)
```
---
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-8886: Limitations is listUsage...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/858#issuecomment-189993774 Hello @kansal I think you just should test the possibility of setting the domainName as null and see what happens. With that tested, code 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: [CLOUDSTACK-8973] Fix create template fro...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1424#discussion_r54362124
--- Diff: server/src/com/cloud/api/ApiResponseHelper.java ---
@@ -1388,7 +1388,7 @@ public TemplateResponse
createTemplateUpdateResponse(ResponseView view, VirtualM
@Override
public List createTemplateResponses(ResponseView
view, VirtualMachineTemplate result, Long zoneId, boolean readyOnly) {
List tvo = null;
-if (zoneId == null || zoneId == -1) {
+if (zoneId == null || zoneId == -1 || result.isCrossZones()) {
--- End diff --
@syed could you make extra test cases for this addition?
---
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 alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1424#discussion_r54362098
--- Diff: server/src/com/cloud/template/TemplateManagerImpl.java ---
@@ -1705,6 +1709,14 @@ public VMTemplateVO
createPrivateTemplateRecord(CreateTemplateCmd cmd, Account t
s_logger.debug("This template is getting created from
other template, setting source template Id to: " + sourceTemplateId);
}
}
+
+
+// ignore passed zoneId if we are using region wide image store
+List stores = _imgStoreDao.findRegionImageStores();
+if (stores != null && stores.size() > 0) {
--- End diff --
Hello @syed
this if condition can be changed to use the Apache's CollectionUtils
isEmpty() method for enhanced readability
https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/CollectionUtils.html#isEmpty(java.util.Collection)
---
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-8865: Adding SR doesn't create...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/876#discussion_r53570532
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2378,6 +2378,23 @@ public boolean maintenanceFailed(final long hostId) {
}
@Override
+public List listAllUpHosts(Type type, Long clusterId, Long
podId, long dcId) {
--- End diff --
Hi @SudharmaJain
could you also write an unit test for this new method you created since it
brings new functionalities to the 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9261: Query to traffic sentine...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1418#issuecomment-186875522 Hi @kansal, can you make a test with this change? Try issuing an request with more than 2048 characters to see if everything still works fine then. --- 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-9104: VM naming convention in ...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1302#discussion_r53554647
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -2030,12 +2030,29 @@ int getReservedCpuMHZ(VirtualMachineTO vmSpec) {
return new String[] {datastoreDiskPath};
}
-// Pair
-private Pair<String, String> composeVmNames(VirtualMachineTO vmSpec) {
-String vmInternalCSName = vmSpec.getName();
-String vmNameOnVcenter = vmSpec.getName();
-if (_instanceNameFlag && vmSpec.getHostName() != null) {
-vmNameOnVcenter = vmSpec.getHostName();
+
+/**
+ * This method gemerate VM name for Vcenter and Cloudstack( when
Hypervisor is VMware).
--- End diff --
Just a little typo in line 2035, "gemerate".
Thanks for taking the time to write the javadoc.
---
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-8897: baremetal:addHost:make h...
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/874#discussion_r52846753
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -680,6 +680,13 @@ public Discoverer getMatchingDiscover(final
Hypervisor.HypervisorType hypervisor
}
}
+if
((hypervisorType.equalsIgnoreCase(HypervisorType.BareMetal.toString( {
+if (hostTags == null || hostTags.size() == 0) {
--- End diff --
Hey @harikrishna-patnala just a little tip on line 684. You could use
Apache Commons' CollectionUtils to make such checks, it does exactly the same
thing but is a little more clean. But that's just a little adjustment.
http://commons.apache.org/proper/commons-collections/javadocs/api-release/org/apache/commons/collections4/CollectionUtils.html#isEmpty(java.util.Collection)
---
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 alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/797#discussion_r52846855
--- 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);
--- End diff --
Hello @maneesha-p
I got a little confused by this if statement. Are you actually doing
anything inside it? If Im not mistaken both variables are local inside this
if-block and won't do anything outside it's scope, is this intended?
---
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-6928: fix issue disk I/O throt...
Github user alexandrelimassantana commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1410#discussion_r52846110 --- Diff: engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java --- @@ -207,10 +211,14 @@ public VolumeInfo moveVolume(VolumeInfo volume, long destPoolDcId, Long destPool // Find a destination storage pool with the specified criteria DiskOffering diskOffering = _entityMgr.findById(DiskOffering.class, volume.getDiskOfferingId()); -; DiskProfile dskCh = new DiskProfile(volume.getId(), volume.getVolumeType(), volume.getName(), diskOffering.getId(), diskOffering.getDiskSize(), diskOffering.getTagsArray(), diskOffering.getUseLocalStorage(), diskOffering.isRecreatable(), null); dskCh.setHyperType(dataDiskHyperType); +dskCh.setBytesReadRate(storageMgr.getDiskBytesReadRate(null, diskOffering)); --- End diff -- Hello @ustcweizhou, I see you have added into multiple methods the lines 217-220, where the only slight change is the first parameter being null or an ServiceOffering instance. I think that in order to avoid repeating the same code, you could extract those lines into the DiskProfile class. That way you can document and test the method and enhance reusability. --- 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-8830 CLONE - VM snapshot fails...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/798#issuecomment-178042092 Hi @maneesha-p I see you did the extraction, but could you also write the method javadoc along with it? As simple as it is, it can help people understand it when using 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8910: The reserved_capacity fi...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/892#issuecomment-177591098 @SudharmaJain could you turn that added piece of code into a method and create some testCases? Manually testing works, but making some unit tests helps a little bit more. --- 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-9251: Fix issue in scale VM to...
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-176851830
@ustcweizhou there are some modifications you could do to make this code a
little more clean and readable.
**1)** You are duplicating code, since two classes have the same code
written. You should reallocate this method to a common superclass.
**2)** You could change lines **84-86** (ScaleVMCmd.java file) from line
comments to the Javadoc's pattern, which can be done much like a block comment,
but with double asterisks /** Javadoc **/
```Java
/**
* I am a Javadoc and this is general information about the method
* @param This is a param documentation.
* @return This is a method's return documentation.
**/
```
**3)** You should use **!details.isEmpty()** instead of **details.size() !=
0**. It better states what you are looking for and it's performance is slightly
better than the comparison you are using.
**4)** It's a good practice to declare the variables as the first
statements of the method, that will help when someone is reading inside
conditionals, the same method written with these changes would look like:
```Java
public Map<String, String> getDetails(){
Map<String, String> customparameterMap = new HashMap<String, String>();
Collection parameterCollection;
Iterator iter;
HashMap<String, String> value;
if (details == null || details.isEmpty())
return customparameterMap;
parameterCollection = details.values();
iter = parameterCollection.iterator();
while (iter.hasNext()) {
value = (HashMap<String, String>)iter.next();
for (String key : value.keySet())
customparameterMap.put(key, value.get(key));
}
return customparameterMap;
}
```
---
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 alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1246#issuecomment-174398371
Hello, there is also something bugging me with this piece of code which
appears on the changed files:
```java
final String gatewayCidr = guestNetwork.getNetworkCidr() == null ?
guestNetwork.getCidr() : guestNetwork.getNetworkCidr();
```
First, it is repeated code, which could turn into a function in the
superclass, since VirtualNetworkApplianceManagerImpl and NetworkHelpImpl are
implementation classes.
Also, you are calling the function getNetworkCidr twice in the event of
it's return value not being null. It's both confusing and not efficient. You
could try writing a separate method for the purpose of correctly setting this
variable, such as:
```java
protected String getValidGatewayCidr(){
String gatewayCidr = guestNetwork.getNetworkCidr();
return gatewayCidr == null ? guestNetwork.getCidr() : null;
}
```
These changes would make the code more understandable, provided that you
could write javadocs on these pieces of 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1302#issuecomment-174396396 @priyankparihar, since you are modifying this method, you could also generate it's javadoc. It would be a nice upgrade from that single line comment placed just before the method signature ( line 2033 ). You could even state your parameter null check in the @parameter field in it. --- 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-8830 CLONE - VM snapshot fails...
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/798#issuecomment-171739474
@maneesha-p you could extract the content of this if-condition into a
method to test the TaskInfo object, making the code a little more reusable and
allowing you to build up an isolated TestCase for this scenario.
```Java
if (info == null || info.getEntityName() == null || info.getName() == null)
{
continue;
}
```
---
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: Removal of DefaultUserAuthenticator empty...
Github user alexandrelimassantana commented on the pull request: https://github.com/apache/cloudstack/pull/1100#issuecomment-158764180 Thanks for that look-up @rafaelweingartner. Yeah that class was very old, it's use in the hierarchy, as of now, was just for forwading an interface and a superclass to it's children. 2015-11-21 20:52 GMT-02:00 Rafael Weingärtner <notificati...@github.com>: > Sure ;) > He is one of my team mates of that project we talked about. > Today was one of the days that we meet and try to clean some code. We > normally do that once a month. > > â > Reply to this email directly or view it on GitHub > <https://github.com/apache/cloudstack/pull/1100#issuecomment-158688747>. > --- 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: Removal of DefaultUserAuthenticator empty...
GitHub user alexandrelimassantana opened a pull request: https://github.com/apache/cloudstack/pull/1100 Removal of DefaultUserAuthenticator empty class. The DefaultUserAuthenticator is an empty class, extending from the AdapterBase and implementing the UserAuthenticator interface. The class is not being used as a marker and it's sole use is to be extended by other UserAuthenticators. Noticing that the class had no purpose, I removed it and made it's children extend from it's superclass and implement it's interface instead. Also, I removed the @Local tags from those classes, since EJB is not used anymore. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack master-lrg-cs-hackday-005 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1100.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 #1100 commit cd57451f4b0ea7e42ad5408486e80f43a55367af Author: alexandre de Limas Santana <alexandre.limas.sant...@gmail.com> Date: 2015-11-21T18:27:07Z Removal of DefaultUserAuthenticator empty class. The DefaultUserAuthenticator is an empty class, extending from the AdapterBase and implementing the UserAuthenticator interface. The class is not being used as a marker and it's sole use is to be extended by other UserAuthenticators. Noticing that the class had no purpose, I removed it and made it's children extend from it's superclass and implement it's interface instead. Also, I removed the @Local tags from those classes, since EJB is not used anymore. --- 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: Removal of class AgentBasedStandaloneCons...
GitHub user alexandrelimassantana opened a pull request: https://github.com/apache/cloudstack/pull/855 Removal of class AgentBasedStandaloneConsoleProxyManager The AgentBasedStandaloneConsoleProxyManager class is neither manually instatiated anywhere in the code nor via Spring. Further checking in the interface which the class implements, shows that the use for classes implementing the EventBus interface relies on instances generated by the Spring framework, which discards the possibility of the class being created by EJB. Being the class useless, it was discarded. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack master-lrg-cs-hackday-007 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/855.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 #855 commit 5470cc4e76c9753b3e2894ff50ff6940443f2bd6 Author: Alexandre de Limas Santana <alexandre.limas.sant...@gmail.com> Date: 2015-09-19T19:57:04Z Removal of class AgentBasedStandaloneConsoleProxyManager The AgentBasedStandaloneConsoleProxyManager class is neither manually instatiated anywhere in the code nor via Spring. Further checking in the interface which the class implements, shows that the use for classes implementing the EventBus interface relies on instances generated by the Spring framework, which discards the possibility of the class being created by EJB. Being the class useless, it was discarded. --- 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: Removal of DefaultUserAuthenticator empty...
GitHub user alexandrelimassantana opened a pull request: https://github.com/apache/cloudstack/pull/852 Removal of DefaultUserAuthenticator empty class. The DefaultUserAuthenticator is an empty class, extending from the AdapterBase and implementing the UserAuthenticator interface. The class is not being used as a marker and it's sole use is to be extended by other UserAuthenticators. Noticing that the class had no purpose, I removed it and made it's children extend from it's superclass and implement it's interface instead. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rafaelweingartner/cloudstack master-lrg-cs-hackday-005 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/852.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 #852 commit 91daba381d95ac4556a60db62799fdeee0da4445 Author: Alexandre de Limas Santana <alexandre.limas.sant...@gmail.com> Date: 2015-09-19T18:30:27Z Removal of DefaultUserAuthenticator empty class. The DefaultUserAuthenticator is an empty class, extending from the AdapterBase and implementing the UserAuthenticator interface. The class is not being used as a marker and it's sole use is to be extended by other UserAuthenticators. Noticing that the class had no purpose, I removed it and made it's children extend from it's superclass and implement it's interface instead. --- 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. ---
