You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by DaanHoogland <gi...@git.apache.org> on 2015/07/09 15:11:06 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/572
CLOUDSTACK-7539: [S3] Parallel deployment makes reference count of a …
…cache in nfs secondary staging store negative(-1)
old review request by Hiroki Ohashi that is planned for 4.5.2
@bhaisaab, @karuturi, @wilderrodrigues please review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack CLOUDSTACK-7539
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/572.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 #572
----
commit 780816ee9f9461f1eaa6bc5133354f99b06cd208
Author: Hiroki Ohashi <hi...@aist.go.jp>
Date: 2014-10-02T08:21:06Z
CLOUDSTACK-7539: [S3] Parallel deployment makes reference count of a cache in nfs secondary staging store negative(-1)
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122236249
figured something like that but had't noticed the wait <:}
@bhaisaab @wido are you fine like with this?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122210319
Since the comment of @karuturi says there is something wrong with the code I don't think we can say LGTM here?
I actually encountered this issue today on a running 4.5.2 deployment build from the 4.5 branch.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122216105
@wido, sounds like you mean to have it in but are not satisfied because of @karuturi 's remark. I will look into adding the notify at some place as I think it makes sense.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/572#discussion_r34861726
--- Diff: engine/storage/cache/src/org/apache/cloudstack/storage/cache/manager/StorageCacheManagerImpl.java ---
@@ -216,15 +223,82 @@ public boolean stop() {
@Override
public DataObject createCacheObject(DataObject data, DataStore store) {
- DataObjectInStore obj = objectInStoreMgr.findObject(data, store);
- if (obj != null && obj.getState() == ObjectInDataStoreStateMachine.State.Ready) {
- s_logger.debug("there is already one in the cache store");
- DataObject dataObj = objectInStoreMgr.get(data, store);
- dataObj.incRefCount();
- return dataObj;
+ DataObject objOnCacheStore;
+ final Object lock;
+ final DataObjectType type = data.getType();
+ final String typeName;
+ final DataStoreRole role = store.getRole();
+ final long storeId = store.getId();
+ final long dataId = data.getId();
+
+ /*
+ * Make sure any thread knows own lock type.
+ */
+ if (type == DataObjectType.TEMPLATE) {
+ lock = templateLock;
+ typeName = "template";
+ } else if (type == DataObjectType.VOLUME) {
+ lock = volumeLock;
+ typeName = "volume";
+ } else if (type == DataObjectType.SNAPSHOT) {
+ lock = snapshotLock;
+ typeName = "snapshot";
+ } else {
+ String msg = "unsupported DataObject comes, then can't acquire correct lock object";
+ throw new CloudRuntimeException(msg);
}
+ s_logger.debug("check " + typeName + " cache entry(id: " + dataId + ") on store(id: " + storeId + ")");
+
+ synchronized (lock) {
+ DataObjectInStore obj = objectInStoreMgr.findObject(data, store);
+ if (obj != null) {
+ State st = obj.getState();
+
+ long miliSeconds = 10000;
+ long timeoutSeconds = 3600;
+ long timeoutMiliSeconds = timeoutSeconds * 1000;
+ Date now = new Date();
+ long expiredEpoch = now.getTime() + timeoutMiliSeconds;
+ Date expiredDate = new Date(expiredEpoch);
+
+ /*
+ * Waiting for completion of cache copy.
+ */
+ while (st == ObjectInDataStoreStateMachine.State.Allocated ||
+ st == ObjectInDataStoreStateMachine.State.Creating ||
+ st == ObjectInDataStoreStateMachine.State.Copying) {
+
+ /*
+ * Threads must release lock within waiting for cache copy and
+ * must be waken up at completion.
+ */
+ s_logger.debug("waiting cache copy completion type: " + typeName + ", id: " + obj.getObjectId() + ", lock: " + lock.hashCode());
+ try {
+ lock.wait(miliSeconds);
+ } catch (InterruptedException e) {}
+ s_logger.debug("waken up");
+
+ now = new Date();
+ if (now.after(expiredDate)) {
+ String msg = "Waiting time exceeds timeout limit(" + timeoutSeconds + " s)";
+ throw new CloudRuntimeException(msg);
+ }
- DataObject objOnCacheStore = store.create(data);
+ obj = objectInStoreMgr.findObject(data, store);
+ st = obj.getState();
+ }
+
+ if (st == ObjectInDataStoreStateMachine.State.Ready) {
+ s_logger.debug("there is already one in the cache store");
+ DataObject dataObj = objectInStoreMgr.get(data, store);
+ dataObj.incRefCount();
+ return dataObj;
--- End diff --
we need to `lock.notifyAll();` before return.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122246019
@karuturi @DaanHoogland @bhaisaab I suggest we merge it into 4.5 as well so that it makes 4.5.2
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/572
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122241816
@DaanHoogland are you pushing this to 4.5 branch as well?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122234984
Its not always safe. http://stackoverflow.com/a/14837457/201514
In our example, the threads unnecessarily wait for 1000 ms if the previous threads exited from a path which doesnt do a notify
updated code looks good to me. :+1:
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-121829053
While LGTM and travis seems to be happy about it, my java kung - {locks and synchroization} -fu skills are not to be trusted
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122246365
will 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122225640
I found documentation on the net [1] that this is fine but I changed the flow anyway
[1]http://stackoverflow.com/questions/7971946/in-java-return-value-within-synchronized-block-seems-like-bad-style-does-it-re
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7539: [S3] Parallel deployment...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/572#issuecomment-122240171
I just tested it on 4.5 management server and my deployment errors are gone. I'm fine with it.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---