Yair Zaslavsky has posted comments on this change. Change subject: engine : Modify Async_tasks table to use CommandEntity ......................................................................
Patch Set 14: (7 comments) http://gerrit.ovirt.org/#/c/26687/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskManager.java: Line 742: List<AsyncTasks> tasksInDForStoragePool = tasksInDbAfterRestart.get(sp.getId()); Line 743: if (tasksInDForStoragePool != null) { Line 744: for (AsyncTasks task : tasksInDForStoragePool) { Line 745: if (!_tasks.containsKey(task.getVdsmTaskId())) { Line 746: coco.removeByVdsmTaskId(task.getVdsmTaskId()); Hi Ravi, 1. This really looks alot cleaner - good job. 2. I think that as a future step in async task mgr and CoCo we should think of a concept in Coco which represents an ID of the unit being polled/checked for status/responsible for convergion (i am pretty bad with names, i hope you get my idea) in favor of abstraction. Line 747: } Line 748: } Line 749: } Line 750: http://gerrit.ovirt.org/#/c/26687/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandCoordinatorImpl.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandCoordinatorImpl.java: Line 136: public void updateCommandStatus(final Guid commandId, final AsyncTaskType taskType, final CommandStatus status) { Line 137: commandsCache.updateCommandStatus(commandId, taskType, status); Line 138: } Line 139: Line 140: public List<AsyncTasks> getAllAsyncTasksFromDb() { Hi, just a thought here, Why not have some class - let's call it (once again, bad with names) CocoAsyncTasksHandler, and have all the async tasks code in it? I know that you already have public methods in Coco that involve with async tasks - maybe worth thinking on my previous comment on how we can abstract it? This way, this CocoAsyncTasksHandler can be a part of handlers class hierarchy in the future, and the code of Coco can be cleaner, IMHO. Besides that the current logic looks ok to me. Line 141: List<AsyncTasks> asyncTasks = DbFacade.getInstance().getAsyncTaskDao().getAll(); Line 142: for (AsyncTasks asyncTask : asyncTasks) { Line 143: copyFromCommandEntity(asyncTask, commandsCache.get(asyncTask.getCommandId())); Line 144: } Line 170: /** Line 171: * This method is always called from with in a transaction Line 172: * @param asyncTask Line 173: */ Line 174: public void saveAsyncTaskToDb(final AsyncTasks asyncTask) { Hi, Instead of having the above comment, why not "wrap it" with a REQUIRED transaction scope? Line 175: DbFacade.getInstance().getAsyncTaskDao().save(asyncTask); Line 176: persistCommand(copyToCommandEntity(asyncTask, commandsCache.get(asyncTask.getCommandId()))); Line 177: } Line 178: Line 178: Line 179: public AsyncTasks getAsyncTaskFromDb(Guid asyncTaskId) { Line 180: AsyncTasks asyncTask = DbFacade.getInstance().getAsyncTaskDao().get(asyncTaskId); Line 181: if (asyncTask != null) { Line 182: copyFromCommandEntity(asyncTask, commandsCache.get(asyncTask.getCommandId())); instead of copyFrom I prefer the term "map" - as someone who worked alot on REST-API, I'm sure when you coded this you got reminded in REST-API mappers, no? :) Line 183: } Line 184: return asyncTask; Line 185: } Line 186: http://gerrit.ovirt.org/#/c/26687/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandsCacheImpl.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandsCacheImpl.java: Line 21: public CommandsCacheImpl() { Line 22: commandMap = CacheProviderFactory.<Guid, CommandEntity> getCacheWrapper(COMMAND_MAP_NAME); Line 23: } Line 24: Line 25: public void initializeCache() { any point of of having this "initializeCache" method not private and called from the CTOR? Line 26: if (!cacheInitialized) { Line 27: synchronized(LOCK) { Line 28: if (!cacheInitialized) { Line 29: List<CommandEntity> cmdEntities = DbFacade.getInstance().getCommandEntityDao().getAll(); http://gerrit.ovirt.org/#/c/26687/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/SPMAsyncTask.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/SPMAsyncTask.java: Line 244: } Line 245: Line 246: protected void RemoveTaskFromDB() { Line 247: try { Line 248: if (coco.removeByVdsmTaskId(getVdsmTaskId()) != 0) { imho != 0 is bad... and doesn't tell much the code reviewer or any future reader of the code what this means. What do you think? Line 249: log.infoFormat("BaseAsyncTask::RemoveTaskFromDB: Removed task {0} from DataBase", getVdsmTaskId()); Line 250: } Line 251: } Line 252: http://gerrit.ovirt.org/#/c/26687/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/interfaces/AsyncTaskCRUDOperations.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/interfaces/AsyncTaskCRUDOperations.java: Line 5: Line 6: import java.util.List; Line 7: Line 8: public interface AsyncTaskCRUDOperations { Line 9: public abstract List<AsyncTasks> getAllAsyncTasksFromDb(); abstract is not needed. Line 10: public abstract void saveAsyncTaskToDb(AsyncTasks asyncTask); Line 11: public abstract AsyncTasks getAsyncTaskFromDb(Guid asyncTaskId); Line 12: public abstract int removeTaskFromDbByTaskId(Guid taskId) throws RuntimeException; Line 13: public abstract AsyncTasks getByVdsmTaskId(Guid vdsmTaskId); -- To view, visit http://gerrit.ovirt.org/26687 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ied038b514f1285b4d46deff022beafd9dcfe5c29 Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
