Michael Kublin has posted comments on this change.
Change subject: core: Quota refactor - QuotaManager
......................................................................
Patch Set 15: I would prefer that you didn't submit this
(17 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 639:
Line 640: QuotaConsumptionParametersWrapper
quotaConsumptionParametersWrapper = new QuotaConsumptionParametersWrapper(this,
Line 641: getReturnValue().getCanDoActionMessages());
Line 642:
quotaConsumptionParametersWrapper.setParameters(getQuotaConsumptionParameters());
Line 643:
These line is proof you don't need QuotaConsumptionParametersWrapper
Line 644: return
quotaConsumptionParametersWrapper.getParameters().isEmpty() ||
getQuotaManager().consume(quotaConsumptionParametersWrapper);
Line 645: }
Line 646:
Line 647: /**
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaManager.java
Line 30: public class QuotaManager {
Line 31: private final static QuotaManager INSTANCE = new QuotaManager();
Line 32: private final static ReentrantReadWriteLock lock = new
ReentrantReadWriteLock();
Line 33: private final static Log log =
LogFactory.getLog(QuotaManager.class);
Line 34: private final static DecimalFormat percentageFormatter = new
DecimalFormat("#.##");
add final
Line 35: private static QuotaManagerAuditLogger quotaManagerAuditLogger =
new QuotaManagerAuditLogger();
Line 36: private final ConcurrentHashMap<Guid, Map<Guid, Quota>>
storagePoolQuotaMap =
Line 37: new ConcurrentHashMap<Guid, Map<Guid, Quota>>();
Line 38: private final ConcurrentMap<Guid, Quota> directQuotaMap = new
ConcurrentHashMap<Guid, Quota>();
Line 39:
Line 40: public static QuotaManager getInstance() {
Line 41: return INSTANCE;
Line 42: }
Line 43:
Why I need get and set for logger? It can be changed? If yes what about races?
Line 44: public static QuotaManagerAuditLogger getQuotaManagerAuditLogger()
{
Line 45: return quotaManagerAuditLogger;
Line 46: }
Line 47:
Line 497:
Line 498: private List<QuotaStorageConsumptionParameter>
filterQuotaStorageConsumptionParameter(List<QuotaConsumptionParameter>
parameters) {
Line 499: List<QuotaStorageConsumptionParameter> list = new
ArrayList<QuotaStorageConsumptionParameter>();
Line 500: for (QuotaConsumptionParameter parameter : parameters) {
Line 501: if (parameter.getParameterType() ==
QuotaConsumptionParameter.ParameterType.STORAGE) {
why casting here?
Line 502: list.add((QuotaStorageConsumptionParameter)parameter);
Line 503: }
Line 504: }
Line 505: return list;
Line 507:
Line 508: private List<QuotaVdsGroupConsumptionParameter>
filterQuotaVdsGroupConsumptionParameter(List<QuotaConsumptionParameter>
parameters) {
Line 509: List<QuotaVdsGroupConsumptionParameter> list = new
ArrayList<QuotaVdsGroupConsumptionParameter>();
Line 510: for (QuotaConsumptionParameter parameter : parameters) {
Line 511: if (parameter.getParameterType() ==
QuotaConsumptionParameter.ParameterType.VDS_GROUP) {
why casting here?
Line 512:
list.add((QuotaVdsGroupConsumptionParameter)parameter);
Line 513: }
Line 514: }
Line 515: return list;
Line 565: double storageRequestPercentage,
Line 566: List<String> canDoActionMessages,
Line 567: AuditLogableBase auditLogableBase) {
Line 568: double storageTotalPercentage = storageUsagePercentage +
storageRequestPercentage;
Line 569:
Method is not understandable. Or one return point, or at every if conditions,
but not both.
Line 570: if (limit == QuotaStorage.UNLIMITED || storageTotalPercentage
<= quota.getThresholdStoragePercentage()) {
Line 571: return true;
Line 572: } else if (storageTotalPercentage <= 100) {
Line 573: quotaManagerAuditLogger.auditLogStorage(auditLogableBase,
Line 693: int newVcpu = vcpuToAdd + quotaVdsGroup.getVirtualCpuUsage();
Line 694:
Line 695: long memLimit = quotaVdsGroup.getMemSizeMB();
Line 696: int cpuLimit = quotaVdsGroup.getVirtualCpu();
Line 697:
Same here: reduce return points
Line 698: if (memLimit == QuotaVdsGroup.UNLIMITED_MEM && cpuLimit ==
QuotaVdsGroup.UNLIMITED_VCPU) { // if both cpu and
Line 699: // mem are unlimited
Line 700: // cache
Line 701: cacheNewValues(quotaVdsGroup, newMemory, newVcpu);
Line 823: }
Line 824:
Line 825: }
Line 826:
Line 827: public boolean
validateAndSetClusterQuota(QuotaConsumptionParametersWrapper parameters) {
Public method, where is readWriteLock
Line 828: List<QuotaVdsGroupConsumptionParameter>
vdsGroupConsumptionParameters =
Line 829:
filterQuotaVdsGroupConsumptionParameter(parameters.getParameters());
Line 830: synchronized
(storagePoolQuotaMap.get(parameters.getAuditLogable().getStoragePool().getId()))
{
Line 831: for (QuotaVdsGroupConsumptionParameter parameter :
vdsGroupConsumptionParameters) {
Line 861: }
Line 862: return true;
Line 863:
Line 864: }
Line 865:
same bug is here
Line 866: public boolean validateAndSetClusterQuota(storage_pool
storagePool,
Line 867: Guid vdsGroupId,
Line 868: Guid quotaId,
Line 869: int vcpu,
Line 1116: * - Quota consumption parameters
Line 1117: */
Line 1118:
Line 1119: private boolean
validateAndCompleteParameters(QuotaConsumptionParametersWrapper parameters) {
Line 1120:
Enums use == instead of equals
Line 1121: if
(QuotaEnforcementTypeEnum.DISABLED.equals(parameters.getAuditLogable().getStoragePool().getQuotaEnforcementType()))
{
Line 1122: return true;
Line 1123: }
Line 1124:
Line 1183:
Line 1184: // In case this param is a QuotaVdsConsumptionParameter - check
that it has a valid
Line 1185: // vds group id which is handled by this quota
Line 1186: private boolean
checkVdsGroupMatchQuota(QuotaConsumptionParametersWrapper parameters,
QuotaConsumptionParameter param) {
Line 1187: Quota quota = param.getQuota();
Why casting here?
Line 1188: QuotaVdsGroupConsumptionParameter paramVds =
(QuotaVdsGroupConsumptionParameter) param;
Line 1189:
Line 1190: if (paramVds.getVdsGroupId() == null) {
Line 1191:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 1185: // vds group id which is handled by this quota
Line 1186: private boolean
checkVdsGroupMatchQuota(QuotaConsumptionParametersWrapper parameters,
QuotaConsumptionParameter param) {
Line 1187: Quota quota = param.getQuota();
Line 1188: QuotaVdsGroupConsumptionParameter paramVds =
(QuotaVdsGroupConsumptionParameter) param;
Line 1189:
You are building parameters, how that happened that at some place inside it can
be null? Why to build parameters with wrong vdsGroupId ?
Line 1190: if (paramVds.getVdsGroupId() == null) {
Line 1191:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 1192: log.errorFormat("Quota Vds parameters from command: {0}
are missing vds group id",
Line 1193:
parameters.getAuditLogable().getClass().getName());
Line 1216:
Line 1217: // In case this param is a QuotaStorageConsumptionParameter -
check that it has a valid
Line 1218: // storage domain id which is handled by this quota
Line 1219: private boolean
checkStoragePoolMatchQuota(QuotaConsumptionParametersWrapper parameters,
QuotaConsumptionParameter param) {
Line 1220: Quota quota = param.getQuota();
Casting?
Line 1221: QuotaStorageConsumptionParameter paramstorage =
(QuotaStorageConsumptionParameter) param;
Line 1222:
Line 1223: if (paramstorage.getStorageDomainId() == null) {
Line 1224:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 1218: // storage domain id which is handled by this quota
Line 1219: private boolean
checkStoragePoolMatchQuota(QuotaConsumptionParametersWrapper parameters,
QuotaConsumptionParameter param) {
Line 1220: Quota quota = param.getQuota();
Line 1221: QuotaStorageConsumptionParameter paramstorage =
(QuotaStorageConsumptionParameter) param;
Line 1222:
Why it is null?
Line 1223: if (paramstorage.getStorageDomainId() == null) {
Line 1224:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 1225: log.errorFormat("Quota storage parameters from command:
{0} are missing storage domain id",
Line 1226:
parameters.getAuditLogable().getClass().getName());
Line 1252: * @param quotaId - quota id
Line 1253: * @return - found quota. null if not found.
Line 1254: */
Line 1255: private Quota fetchQuotaFromDB(Guid quotaId) {
Line 1256: // if the id is valid
How these can be? quataId == null || quotaId == Guid.Empty?
It is internal method and you are performing a checks before.
Line 1257: if (quotaId != null && quotaId != Guid.Empty) {
Line 1258: if (!directQuotaMap.containsKey(quotaId)) {
Line 1259: // cache in direct quota map
Line 1260: Quota quota = getQuotaDAO().getById(quotaId);
Line 1253: * @return - found quota. null if not found.
Line 1254: */
Line 1255: private Quota fetchQuotaFromDB(Guid quotaId) {
Line 1256: // if the id is valid
Line 1257: if (quotaId != null && quotaId != Guid.Empty) {
If you trying to synchronized directQuotaMap , all operations on it should be
inside CS protected by locks
Line 1258: if (!directQuotaMap.containsKey(quotaId)) {
Line 1259: // cache in direct quota map
Line 1260: Quota quota = getQuotaDAO().getById(quotaId);
Line 1261: if (quota == null) {
Line 1264: }
Line 1265: lock.readLock().lock();
Line 1266: try {
Line 1267: directQuotaMap.put(quotaId, quota);
Line 1268: // cache in storage-pool->quota map
First of all lock inside of private method is not obvious, the method can be
used anywhere can cause to deadlocks.
Also storagePoolQuotaMap is ConcurrentHashMap if it is inside a lock switch to
regular HashMap
Line 1269:
storagePoolQuotaMap.putIfAbsent(quota.getStoragePoolId(), new HashMap<Guid,
Quota>());
Line 1270:
storagePoolQuotaMap.get(quota.getStoragePoolId()).put(quotaId, quota);
Line 1271: } finally {
Line 1272: lock.readLock().unlock();
--
To view, visit http://gerrit.ovirt.org/8776
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb100467a55b26e4219d1a2562da86b81ffdc032
Gerrit-PatchSet: 15
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <oma...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Sharad Mishra <snmis...@linux.vnet.ibm.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: ofri masad <oma...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
