-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed wrong synchronization of threads in throttling counting #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my side, it is necessary to do:
- read contribution notes - https://openhubframework.atlassian.net/wiki/display/OHF/Contribution
- read contribution rules and principles - https://openhubframework.atlassian.net/wiki/display/OHF/Rules+and+principles
- for all java based files please change version to 2.0.0 (next release version will be 2.0.0)
- please avoid to use full author tag (do not add e-mail address)
- it is necessary to rebase these commits on to develop and solve some conflicts
- please improve title and description of this PR
- it is possible to squash all commits together?
After I can continue in review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add summary of what is going on in this task (https://openhubframework.atlassian.net/browse/OHFJIRA-14) - we know problem and it's necessary to add task description, what is implemented
package org.openhubframework.openhub.common.synchronization; | ||
|
||
/** | ||
* Method {@link #doInSynchronizationPart()} in this interface can be synchronized by one one value in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mistake "one one"
import java.util.concurrent.ConcurrentHashMap; | ||
|
||
/** | ||
* Executor for {@link SynchronizationBlock} which synchronized more then one threads by one value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which -> that
* Executor for {@link SynchronizationBlock} which synchronized more then one threads by one value. | ||
* <p> | ||
* For synchronization by one value call {@link #execute(SynchronizationBlock, String, Object)}, where in method | ||
* {@link SynchronizationBlock#doInSynchronizationPart()} are code which will be by synchronized by value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which -> that + will be synchronized by value
* Method will be synchronized by one value calling | ||
* {@link SynchronizationExecutor#execute(SynchronizationBlock, String, Object)}. | ||
*/ | ||
protected abstract void doInSynchronizationPartNoResult(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the class has name SynchronizationNoResultBlock then method could be shorter, for example doInSynchro or synchroBlock ...
* <p> | ||
* For synchronization by one value call {@link #execute(SynchronizationBlock, String, Object)}, where in method | ||
* {@link SynchronizationBlock#doInSynchronizationPart()} are code which will be by synchronized by value. | ||
* </p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little bit more
:)
Maybe we should consider new PR in Camel Core: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small changes are necessary (reformat code) and consider if lazy loaded singleton is necessary. After clarification it can be approved (merged).
* | ||
* @return instance of this class | ||
*/ | ||
public synchronized static SynchronizationExecutor getInstance() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synchronization of lazy loaded singleton is broken. Please you static eager initialization - http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html?page=2.
@@ -45,81 +43,60 @@ | |||
|
|||
private static final int DUMP_PERIOD = 60; | |||
|
|||
private static final String SYNC_THROTTLING_TYPE = "SYNCHRONIZATION_THROTTLING"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe THROTTLING_SYNCHRONIZATION
is better name.
|
||
long now = DateTime.now().getMillis(); | ||
long from = now - (interval * 1000); | ||
long now = DateTime.now().getMillis(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use spaces for indent, no tabs - see https://openhubframework.atlassian.net/wiki/display/OHF/Source+code+conventions#Sourcecodeconventions-Syntaxrules.
ef48f1c
to
f65747d
Compare
Coverage increased (+0.3%) to 70.893% when pulling f65747d774d276a31bace8666e73491127e9d179 on feature/OHFJIRA-14 into 716fd89 on develop. |
Issue: OHFJIRA-14
f65747d
to
b30aaf9
Compare
No description provided.