ResourceBean volatile increment of int variable #7
Comments
|
I realized there are other places where numeric volatile variables are incremented:
|
|
Be careful to understand the usage pattern before changing these. For example, The same is true of |
|
Hi, I still think it would have been better to change those, the code would "scream" its intentions better. Even if you stick to this pattern, it's at least safe to document the method as not Tread-safe, since you depend on an external lock to ensure atomicity. Vlad On Monday, January 20, 2014 3:49 AM, Brett Wooldridge notifications@github.com wrote: Be careful to understand the usage pattern before changing these. For example, JdbcPooledConnection is only ever accessed by a single thread at one time. usageCount will never be incremented/decremented in a multi-threaded context, so AtomicInteger is unnecessary. usageCount is volatile merely to ensure correct visibility of the count once that connection is returned to the pool and handed to another thread. This is an appropriate use of volatile. |
|
Hi Brett, Sorry for being annoying on this one, but after you mentioned the intended usage, I wanted to make sure all paths are guarded by locks, so please confirm this too: JdbcPooledConnection.usageCount is decremented in release() method, but that one is not guarded by any external lock. Am I missing something? Or should we make it an AtomicInteger as I proposed in the very first place? Vlad On Monday, January 20, 2014 6:51 AM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, I still think it would have been better to change those, the code would "scream" its intentions better. Even if you stick to this pattern, it's at least safe to document the method as not Tread-safe, since you depend on an external lock to ensure atomicity. Vlad On Monday, January 20, 2014 3:49 AM, Brett Wooldridge notifications@github.com wrote: Be careful to understand the usage pattern before changing these. For example, JdbcPooledConnection is only ever accessed by a single thread at one time. usageCount will never be incremented/decremented in a multi-threaded context, so AtomicInteger is unnecessary. usageCount is volatile merely to ensure correct visibility of the count once that connection is returned to the pool and handed to another thread. This is an appropriate use of volatile. |
|
Vlad, release() cannot be called concurrently: connections taken out of the pool I nevertheless agree that this is rather messy. OTOH, mostly (only?) the Ludovic On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea notifications@github.comwrote:
|
|
Hi, Thanks for clarifying this subject. I stumbled on it while investigating the resource code. Indeed, there is no easy way to have both the old way and support the Codahale Metrics too. Right now I am integrating their Histogram into the old ManagementRegistrar and see how it goes. When I have something working, I'll just send you, guys a patch, for code reviewing, rather than commiting/pull-request. I started only with a Histrogram, because that's what I felt BTM is missing at the moment. Replacing the current BTM JMX support and replacing it with Codahale is tough if you don;t want to depend on this lib. If the client doesn't provide this dependency, he may not have any JMX support then. Is this really desirable? Vlad On Monday, January 20, 2014 11:44 AM, lorban notifications@github.com wrote: Vlad, release() cannot be called concurrently: connections taken out of the pool I nevertheless agree that this is rather messy. OTOH, mostly (only?) the Ludovic On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea notifications@github.comwrote:
|
|
If somehow two threads called Connection.close() at the same time on the same instance, one of two things would happen:
If (2) occurs, and the count gets off in the positive direction, it will be logged in the Don't get me wrong, I'm all for |
|
I find it perfectly fair to have to add an extra dep to get any kind of Here's my mental plan of what I would do:
As Brett pointed out, the only goal of that service is to allow running BTM On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea notifications@github.comwrote:
|
|
Hi, I am glad you are open to refactror the whole JMX implementation, rather than patching it with new features. Keep in tough. Vlad On Monday, January 20, 2014 12:22 PM, lorban notifications@github.com wrote: I find it perfectly fair to have to add an extra dep to get any kind of Here's my mental plan of what I would do:
As Brett pointed out, the only goal of that service is to allow running BTM On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea notifications@github.comwrote:
|
|
Hi, I managed to integrate a Codahale Timer using a custom factory adapting their codebase to a simple integration interface. There is one aspect you need to be aware of. The current JMX naming is hard to be supported, and we might end up adopting a new naming convention. This is due to the current Codahale JMX naming support, which is rather trivial, and there is no way to customize it, unless we open a ticket in their project. This is how a histrogram is registered: final ObjectName objectName = createName("histograms", name); where: private ObjectName createName(String type, String name) { So the "this.name" is the domain part, while the "name" variable is the current resource name. This gives us to option:
So, instead of: "bitronix.tm:type=JDBC,UniqueName=" + ManagementRegistrar.makeValidName(getUniqueName()); you will get: "bitronix.tm.jdbc.datasource + ManagementRegistrar.makeValidName(getUniqueName()) + :type=timers,name=waitTime"; Is this something you're willing to accept or compromise? Vlad So, the domain is flexible Vlad On Monday, January 20, 2014 12:28 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, I am glad you are open to refactror the whole JMX implementation, rather than patching it with new features. Keep in tough. Vlad On Monday, January 20, 2014 12:22 PM, lorban notifications@github.com wrote: I find it perfectly fair to have to add an extra dep to get any kind of Here's my mental plan of what I would do:
As Brett pointed out, the only goal of that service is to allow running BTM On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea notifications@github.comwrote:
|
|
I wouldn't bother about maintaining any kind of backward compatibility I've seen this simple example in the doc: final JmxReporter reporter = and thought that would be good enough without any kind of customization. Why do you think we should make more effort than this? On Mon, Jan 20, 2014 at 8:25 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, Actually that's more or less how I started the Jmx registry, but then you need to think of Transaction and connection Jmx resources having a lifespan that's shorter than of a Datasource. For all of those we need both register and unregister, so a naming convention is required to know which resource to close and unregister, on resource disposal. Next I'll try replacing a current resource implementation with same Gauges, for things like max or min connection. On Monday, January 20, 2014 9:43 PM, lorban notifications@github.com wrote: I wouldn't bother about maintaining any kind of backward compatibility I've seen this simple example in the doc: final JmxReporter reporter = and thought that would be good enough without any kind of customization. Why do you think we should make more effort than this? On Mon, Jan 20, 2014 at 8:25 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, Well, connections are acquired serially, because that is lock guarded. Connections are confined to threads as well, so you examples are safe. My worst case scenario implies all these operations combined. Let's assume we have 3 threads, one acquiring and 2 releasing. If all those threads happen to write the usageCount at the very same moment, then we might end up loosing some values. I should have drawn an image, because it's hard to picture it in words, but my use case it's similar to the databases lost update phenomena, when one thread updates a new value based on a 'previous' version of the data, that has changed in between the read and write. Does it only make sense in my head? On Monday, January 20, 2014 10:00 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Actually that's more or less how I started the Jmx registry, but then you need to think of Transaction and connection Jmx resources having a lifespan that's shorter than of a Datasource. For all of those we need both register and unregister, so a naming convention is required to know which resource to close and unregister, on resource disposal. Next I'll try replacing a current resource implementation with same Gauges, for things like max or min connection. On Monday, January 20, 2014 9:43 PM, lorban notifications@github.com wrote: I wouldn't bother about maintaining any kind of backward compatibility I've seen this simple example in the doc: final JmxReporter reporter = and thought that would be good enough without any kind of customization. Why do you think we should make more effort than this? On Mon, Jan 20, 2014 at 8:25 PM, vladmihalcea notifications@github.comwrote:
|
|
The lost update can't happen since usageCount is not shared between connections, being a connection bound variable, so I think it's fine. Case closed. On Tuesday, January 21, 2014 4:59 AM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Well, connections are acquired serially, because that is lock guarded. Connections are confined to threads as well, so you examples are safe. My worst case scenario implies all these operations combined. Let's assume we have 3 threads, one acquiring and 2 releasing. If all those threads happen to write the usageCount at the very same moment, then we might end up loosing some values. I should have drawn an image, because it's hard to picture it in words, but my use case it's similar to the databases lost update phenomena, when one thread updates a new value based on a 'previous' version of the data, that has changed in between the read and write. Does it only make sense in my head? On Monday, January 20, 2014 10:00 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Actually that's more or less how I started the Jmx registry, but then you need to think of Transaction and connection Jmx resources having a lifespan that's shorter than of a Datasource. For all of those we need both register and unregister, so a naming convention is required to know which resource to close and unregister, on resource disposal. Next I'll try replacing a current resource implementation with same Gauges, for things like max or min connection. On Monday, January 20, 2014 9:43 PM, lorban notifications@github.com wrote: I wouldn't bother about maintaining any kind of backward compatibility I've seen this simple example in the doc: final JmxReporter reporter = and thought that would be good enough without any kind of customization. Why do you think we should make more effort than this? On Mon, Jan 20, 2014 at 8:25 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, Related to replacing the current JMX support with Metrics, I think we should also keep the current register/unregister support. That's because Metrics are only a part of the whole JMX monitoring. You also want operations and attributes, not just metrics. For example the PoolingDataSource would still require these: public void reset() throws Exception; And Metrics don't support those, since it would be beyond of the lib scope. Vlad On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad mih_vlad@yahoo.com wrote: The lost update can't happen since usageCount is not shared between connections, being a connection bound variable, so I think it's fine. Case closed. On Tuesday, January 21, 2014 4:59 AM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Well, connections are acquired serially, because that is lock guarded. Connections are confined to threads as well, so you examples are safe. My worst case scenario implies all these operations combined. Let's assume we have 3 threads, one acquiring and 2 releasing. If all those threads happen to write the usageCount at the very same moment, then we might end up loosing some values. I should have drawn an image, because it's hard to picture it in words, but my use case it's similar to the databases lost update phenomena, when one thread updates a new value based on a 'previous' version of the data, that has changed in between the read and write. Does it only make sense in my head? On Monday, January 20, 2014 10:00 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Actually that's more or less how I started the Jmx registry, but then you need to think of Transaction and connection Jmx resources having a lifespan that's shorter than of a Datasource. For all of those we need both register and unregister, so a naming convention is required to know which resource to close and unregister, on resource disposal. Next I'll try replacing a current resource implementation with same Gauges, for things like max or min connection. On Monday, January 20, 2014 9:43 PM, lorban notifications@github.com wrote: I wouldn't bother about maintaining any kind of backward compatibility I've seen this simple example in the doc: final JmxReporter reporter = and thought that would be good enough without any kind of customization. Why do you think we should make more effort than this? On Mon, Jan 20, 2014 at 8:25 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, This is how the metrics look like in my current project. I think I will address the waitTime and the connection pool histogram in the reported issue. This will add the CodaHale Metrics to bitronix, but will not touch the existing JMX implementation, which it's still required for Operations/Attributes. When I have a final version, I will send you a pull-request, or a patch, whichever you prefer for some code reviewing. Refactoring JMX is more time and brain intensive, so it may be addressed in a separate issue. Are you comfortable with my proposed approach? Vlad On Tuesday, January 21, 2014 11:31 AM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Related to replacing the current JMX support with Metrics, I think we should also keep the current register/unregister support. That's because Metrics are only a part of the whole JMX monitoring. You also want operations and attributes, not just metrics. For example the PoolingDataSource would still require these: public void reset() throws Exception; And Metrics don't support those, since it would be beyond of the lib scope. Vlad On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad mih_vlad@yahoo.com wrote: The lost update can't happen since usageCount is not shared between connections, being a connection bound variable, so I think it's fine. Case closed. On Tuesday, January 21, 2014 4:59 AM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Well, connections are acquired serially, because that is lock guarded. Connections are confined to threads as well, so you examples are safe. My worst case scenario implies all these operations combined. Let's assume we have 3 threads, one acquiring and 2 releasing. If all those threads happen to write the usageCount at the very same moment, then we might end up loosing some values. I should have drawn an image, because it's hard to picture it in words, but my use case it's similar to the databases lost update phenomena, when one thread updates a new value based on a 'previous' version of the data, that has changed in between the read and write. Does it only make sense in my head? On Monday, January 20, 2014 10:00 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, Actually that's more or less how I started the Jmx registry, but then you need to think of Transaction and connection Jmx resources having a lifespan that's shorter than of a Datasource. For all of those we need both register and unregister, so a naming convention is required to know which resource to close and unregister, on resource disposal. Next I'll try replacing a current resource implementation with same Gauges, for things like max or min connection. On Monday, January 20, 2014 9:43 PM, lorban notifications@github.com wrote: I wouldn't bother about maintaining any kind of backward compatibility I've seen this simple example in the doc: final JmxReporter reporter = and thought that would be good enough without any kind of customization. Why do you think we should make more effort than this? On Mon, Jan 20, 2014 at 8:25 PM, vladmihalcea notifications@github.comwrote:
|
|
A pull request is fine. It is preferred if you squash your working commits Brett On Tue, Jan 21, 2014 at 11:31 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, I've only used Git and GitHub for my own repo, and submitted simple pull-requests from time to time. Usually that's how I've done it, using a single commit. I am not sure how Git manages to take a bunch of commits and create a smart pull-request, so I'll stick to basics, as you mentioned it. Vlad On Tuesday, January 21, 2014 4:56 PM, Brett Wooldridge notifications@github.com wrote: A pull request is fine. It is preferred if you squash your working commits Brett On Tue, Jan 21, 2014 at 11:31 PM, vladmihalcea notifications@github.comwrote:
|
|
When working with other projects, I usually just do a git rebase -i HEAD~x Brett On Wed, Jan 22, 2014 at 12:02 AM, vladmihalcea notifications@github.comwrote:
|
|
Hi Brett, I have the changes packed. I haven't done any commit on my branch, since I wanted to have all changes in one batch (silly but I've started using Git for just 3 months, and it was more through GitHub). Since, I haven't yet committed I could go anyway it's easier for you to review it. In GitHub UI I usually commit and sync (which pulls and pushes in one shoot). Then, I can send you a pull-request. Should I try doing other way through Git command line? Is there any benefit compared to the GitHub way? Vlad On Tuesday, January 21, 2014 5:09 PM, Brett Wooldridge notifications@github.com wrote: When working with other projects, I usually just do a git rebase -i HEAD~x Brett On Wed, Jan 22, 2014 at 12:02 AM, vladmihalcea notifications@github.comwrote:
|
|
What OS are you using? A but unrelated but ... first thing I would do, if Windows or Mac, I would [image: Inline image 1] If you haven't committed anything, even locally, it's pretty easy. Just In the future, I would recommend creating a branch (sometimes called a Now, for squashing your commits into one single commit ... read this: http://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/ That doesn't really apply right now, because you haven't committed On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, I tried to pull the request but it added the commit to the one I'd already sent you for AtomicInteger. Vlad On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge notifications@github.com wrote: What OS are you using? A but unrelated but ... first thing I would do, if Windows or Mac, I would [image: Inline image 1] If you haven't committed anything, even locally, it's pretty easy. Just In the future, I would recommend creating a branch (sometimes called a Now, for squashing your commits into one single commit ... read this: http://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/ That doesn't really apply right now, because you haven't committed On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea notifications@github.comwrote:
|
|
It looks reviewable in that form. After any changes as a result of the It's midnight here in Tokyo, but I'll take a look at the changes in the Thanks, On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, I decided to rollback those 2 commits and only send a pull request with the metrics. Vlad On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge notifications@github.com wrote: It looks reviewable in that form. After any changes as a result of the It's midnight here in Tokyo, but I'll take a look at the changes in the Thanks, On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, I rolled-back the AtomicInteger thing, but seems like GitHub still keeps the revered commits in the pull-request list. Vlad On Wednesday, January 22, 2014 4:49 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, I decided to rollback those 2 commits and only send a pull request with the metrics. Vlad On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge notifications@github.com wrote: It looks reviewable in that form. After any changes as a result of the It's midnight here in Tokyo, but I'll take a look at the changes in the Thanks, On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea notifications@github.comwrote:
|
|
Hi, I see that travis failed on jdk7, but succeeded on jdk6. That's strange, since I tested it with java7, after changing mvn -P java7 clean install Can you replicate the failed test? Vlad On Wednesday, January 22, 2014 5:03 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, I rolled-back the AtomicInteger thing, but seems like GitHub still keeps the revered commits in the pull-request list. Vlad On Wednesday, January 22, 2014 4:49 PM, Mihalcea Vlad mih_vlad@yahoo.com wrote: Hi, I decided to rollback those 2 commits and only send a pull request with the metrics. Vlad On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge notifications@github.com wrote: It looks reviewable in that form. After any changes as a result of the It's midnight here in Tokyo, but I'll take a look at the changes in the Thanks, On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea notifications@github.comwrote:
|
|
The build does not fail on my computer. I re-ran the TravisCI build and it On Thu, Jan 23, 2014 at 8:06 AM, Brett Wooldridge <
|
|
Hi, I submitted a new pull-request and closed the old one. I tried the rebase but doesn't seem to work when you have reverts too. So I had to squash using a new branch, and recreate my master. I applied your review suggestions and changed the in-pool-size calling logic as it's value retrieving logic. I hope I got it right. I'll have to read a git book before attempting doing more fixes :D Vlad On Thursday, January 23, 2014 4:02 AM, Brett Wooldridge notifications@github.com wrote: The build does not fail on my computer. I re-ran the TravisCI build and it On Thu, Jan 23, 2014 at 8:06 AM, Brett Wooldridge <
|
The increment is not an atomic operation, even if we use a volatile variable.
private volatile transient int createdResourcesCounter;
public int incCreatedResourcesCounter() {
return this.createdResourcesCounter++;
}
This could be fixed by replacing the int variable with an AtomicInteger.
The text was updated successfully, but these errors were encountered: