Alissa Bonas has posted comments on this change.
Change subject: core: Locate data source in a loop
......................................................................
Patch Set 5: (4 inline comments)
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacadeLocator.java
Line 33:
Line 34: // The facade singleton:
Line 35: private static DbFacade dbFacade;
Line 36:
Line 37: public synchronized static DbFacade getDbFacade() {
great, it looks much better in the new patchset.
Line 38: // Do nothing if the facade has already been created:
Line 39: if (dbFacade != null) {
Line 40: return dbFacade;
Line 41: }
Line 51: // to wait:
Line 52: long timeStarted = System.currentTimeMillis();
Line 53:
Line 54: DataSource datasource = null;
Line 55: for (;;) {
I think that evaluating 2 long vars, even twice is not that bad in this
situation. I think it's better and more readable than an endless loop. But
perhaps it's a matter of style. Maybe other reviewers can contribute here their
opinions too.
Line 56: // Try to locate the data source:
Line 57: datasource =
EjbUtils.findResource(ContainerManagedResourceType.DATA_SOURCE);
Line 58: if (datasource != null) {
Line 59: break;
Line 82: Thread.sleep(checkInterval);
Line 83: }
Line 84: catch (InterruptedException exception) {
Line 85: log.warnFormat(
Line 86: "Interrupted while waiting for data source.",
exception
Well, the general recommended best practice is actually to call :
Thread.currentThread().interrupt();
But of course it is specific in each case...
Just check this to consider it again for example:
http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html
Line 87: );
Line 88: }
Line 89: }
Line 90:
Line 114: try {
Line 115: props =
ResourceUtils.loadProperties(DbFacadeLocator.class,
ENGINE_DB_ENGINE_PROPERTIES);
Line 116: }
Line 117: catch (IOException exception) {
Line 118: throw new IllegalStateException(
Is this exception logged somewhere? In which log and by which component?
Same question about all exceptions thrown in the methods below...
Line 119: "Can't load properties from resource \"" +
Line 120: ENGINE_DB_ENGINE_PROPERTIES + "\".", exception
Line 121: );
Line 122: }
--
To view, visit http://gerrit.ovirt.org/10189
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I72c99c61d05e8a1619c7d1fb70af956d1050eb3a
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
