ResourceBean volatile increment of int variable about btm HOT 28 CLOSED

I realized there are other places where numeric volatile variables are incremented:

• JdbcPooledConnection.usageCount
  Maybe an umbrella issue should be created to fix them all.

from btm.

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.

The same is true of ResourceBean.createdResourcesCounter. Because it is only called from the JdbcPooledConnection constructor, and the constructor is only called from XAPool.grow() which is synchronized on the poolGrowthShrinkLock, there will never be two threads incrementing the value at one time. volatile merely ensures cross-thread visibility.

from btm.

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 [email protected] 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.
The same is true of ResourceBean.createdResourcesCounter. Because it is only called from the JdbcPooledConnection constructor, and the constructor is only called from XAPool.grow() which is synchronized on the poolGrowthShrinkLock, there will never be two threads incrementing the value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

from btm.

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 [email protected] 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 [email protected] 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.
The same is true of ResourceBean.createdResourcesCounter. Because it is only called from the JdbcPooledConnection constructor, and the constructor is only called from XAPool.grow() which is synchronized on the poolGrowthShrinkLock, there will never be two threads incrementing the value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

from btm.

Vlad,

release() cannot be called concurrently: connections taken out of the pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]
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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it is
only called from the JdbcPooledConnection constructor, and the constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32734381
.

from btm.

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 [email protected] wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]
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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it is
only called from the JdbcPooledConnection constructor, and the constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32734381
.

—
Reply to this email directly or view it on GitHub.

from btm.

JdbcPooledConnection.release() is only called by Connection.close() (through the proxy class). As XA does not permit two threads to share a connection at the same time, personally I do not think it needs to be protected -- two user threads should never be calling Connection.close() on the same connection at the same time. If the user's code is regularly calling our connection from multiple threads at the same time, something somewhere is going to blow-up (as it should).

If somehow two threads called Connection.close() at the same time on the same instance, one of two things would happen:

1. Both would decrement usageCount, or
2. Only one would successfully decrement usageCount (the other's write would be overwritten)

If (2) occurs, and the count gets off in the positive direction, it will be logged in the JdbcPooledConnection.close() method eventually. If (1) happens, well if it makes you feel more comfortable you could add a check in release() that logs at ERROR level if usageCount ever goes negative there.

Don't get me wrong, I'm all for AtomicInteger when there is the possibility of multiple-threads in the code path. But when we have guarantees (whether lock-based or semantically) of single-threadedness, volatile should be sufficient for cross-thread visibility.

from btm.

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep, no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]
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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32746297
.

from btm.

Hi,

I am glad you are open to refactror the whole JMX implementation, rather than patching it with new features.
This is the right thing to do and I am going to do my best to properly design it.
I have never designed for making sure JIT always inlines my code, but I think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected] wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep, no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]
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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32746297
.

—
Reply to this email directly or view it on GitHub.

from btm.

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName), objectName);

where:

private ObjectName createName(String type, String name) {
            try {
                return new ObjectName(this.name, "name", name);
            } catch (MalformedObjectNameException e) {
                try {
                    return new ObjectName(this.name, "name", ObjectName.quote(name));
                } catch (MalformedObjectNameException e1) {
                    LOGGER.warn("Unable to register {} {}", type, name, e1);
                    throw new RuntimeException(e1);
                }
            }
}

So the "this.name" is the domain part, while the "name" variable is the current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource, jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where timer is the default "type", and the name is waittime.

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 [email protected] wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather than patching it with new features.
This is the right thing to do and I am going to do my best to properly design it.
I have never designed for making sure JIT always inlines my code, but I think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected] wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep, no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]
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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32746297
.

—
Reply to this email directly or view it on GitHub.

from btm.

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]
wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32789034
.

from btm.

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.
But that's easy knowing that you don't mind changing the naming convention.

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 [email protected] wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]
wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32789034
.

—
Reply to this email directly or view it on GitHub.

from btm.

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 [email protected] 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.
But that's easy knowing that you don't mind changing the naming convention.

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 [email protected] wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]
wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32789034
.

—
Reply to this email directly or view it on GitHub.

from btm.

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 [email protected] 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 [email protected] 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.
But that's easy knowing that you don't mind changing the naming convention.

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 [email protected] wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]
wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32789034
.

—
Reply to this email directly or view it on GitHub.

from btm.

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;
public boolean isDisabled();
public void setDisabled(boolean disabled);

And Metrics don't support those, since it would be beyond of the lib scope.
In my opinion, I think Bitronix should offer JMX register/unregister of any MBean, be it a Metric or an Operation.
So, BTM should have such service, probably by refactoring the current ManagementRegistrar codebase.
Some of the current MBeans, displaying static data (min size, max size) or attributes (disbaled) or operations (reset) are useful.
Metrics like inPoolSize, totalPoolSize, waitTime are better represented using the Codahale Metrics adaptor.
For BTM all of those are just MBean that it register and unregister as needed.

Vlad

On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad [email protected] 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 [email protected] 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 [email protected] 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.
But that's easy knowing that you don't mind changing the naming convention.

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 [email protected] wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]
wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32789034
.

—
Reply to this email directly or view it on GitHub.

from btm.

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 [email protected] 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;
public boolean isDisabled();
public void setDisabled(boolean disabled);

And Metrics don't support those, since it would be beyond of the lib scope.
In my opinion, I think Bitronix should offer JMX register/unregister of any MBean, be it a Metric or an Operation.
So, BTM should have such service, probably by refactoring the current ManagementRegistrar codebase.
Some of the current MBeans, displaying
static data (min size, max size) or attributes (disbaled) or operations (reset) are useful.
Metrics like inPoolSize, totalPoolSize, waitTime are better represented using the Codahale Metrics adaptor.
For BTM all of those are just MBean that it register and unregister as needed.

Vlad

On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad [email protected] 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 [email protected] 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 [email protected] 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.
But that's easy knowing that you don't mind changing the naming convention.

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 [email protected] wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]
wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such, you
have to stay careful to keep very small methods so that the JIT is be able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]
wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?) the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea [email protected]:

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 [email protected]

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing the
value at one time. volatile merely ensures cross-thread visibility.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32789034
.

—
Reply to this email directly or view it on GitHub.

from btm.

A pull request is fine. It is preferred if you squash your working commits
into a single commit when you think it is ready for review.

Brett

On Tue, Jan 21, 2014 at 11:31 PM, vladmihalcea [email protected]:

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 [email protected]
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;
public boolean isDisabled();
public void setDisabled(boolean disabled);

And Metrics don't support those, since it would be beyond of the lib scope.
In my opinion, I think Bitronix should offer JMX register/unregister of
any MBean, be it a Metric or an Operation.
So, BTM should have such service, probably by refactoring the current
ManagementRegistrar codebase.
Some of the current MBeans, displaying
static data (min size, max size) or attributes (disbaled) or operations
(reset) are useful.
Metrics like inPoolSize, totalPoolSize, waitTime are better represented
using the Codahale Metrics adaptor.
For BTM all of those are just MBean that it register and unregister as
needed.

Vlad

On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad [email protected]
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 [email protected]
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 [email protected]
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.
But that's easy knowing that you don't mind changing the naming convention.

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 [email protected]
wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]

wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's
   related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such,
you
have to stay careful to keep very small methods so that the JIT is be
able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]

wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?)
the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea <
[email protected]>wrote:

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 <
[email protected]>

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing
the
value at one time. volatile merely ensures cross-thread visibility.
�
Reply to this email directly or view it on GitHub.

�
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

�
Reply to this email directly or view it on GitHub.

�
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

�
Reply to this email directly or view it on GitHub.

�
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32789034>
.

�
Reply to this email directly or view it on GitHub.

�
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32889451
.

from btm.

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 [email protected] wrote:

A pull request is fine. It is preferred if you squash your working commits
into a single commit when you think it is ready for review.

Brett

On Tue, Jan 21, 2014 at 11:31 PM, vladmihalcea [email protected]:

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 [email protected]
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;
public boolean isDisabled();
public void setDisabled(boolean disabled);

And Metrics don't support those, since it would be beyond of the lib scope.
In my opinion, I think Bitronix should offer JMX register/unregister of
any MBean, be it a Metric or an Operation.
So, BTM should have such service, probably by refactoring the current
ManagementRegistrar codebase.
Some of the current MBeans, displaying
static data (min size, max size) or attributes (disbaled) or operations
(reset) are useful.
Metrics like inPoolSize, totalPoolSize, waitTime are better represented
using the Codahale Metrics adaptor.
For BTM all of those are just MBean that it register and unregister as
needed.

Vlad

On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad [email protected]
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 [email protected]
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 [email protected]
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.
But that's easy knowing that you don't mind changing the naming convention.

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 [email protected]
wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 [email protected]

wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation, rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]
wrote:

I find it perfectly fair to have to add an extra dep to get any kind of
monitoring support, and that's exactly what I had in mind: no extra dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's
   related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics API,
   that service just needs to be a thin wrapper around Metrics - there's no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow running
BTM
without the Metrics lib, not create any kind of abstraction. As such,
you
have to stay careful to keep very small methods so that the JIT is be
able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea [email protected]:

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 [email protected]

wrote:

Vlad,

release() cannot be called concurrently: connections taken out of the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?)
the
JMX monitoring stuff has a poorly designed concurrency access. That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea <
[email protected]>wrote:

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 <
[email protected]>

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing
the
value at one time. volatile merely ensures cross-thread visibility.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32789034>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32889451
.

—
Reply to this email directly or view it on GitHub.

from btm.

When working with other projects, I usually just do a git rebase -i HEAD~x
in my own cloned repo to squash them in the way I want, then push them,
then submit the pull request. If you've already pushed them to your own
repo, as I tend to do, it is OK. Just squash them locally then do a git
push --force to your repo and it will rewrite history. If you need any
help, just ask.

Brett

On Wed, Jan 22, 2014 at 12:02 AM, vladmihalcea [email protected]:

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 <
[email protected]> wrote:

A pull request is fine. It is preferred if you squash your working commits
into a single commit when you think it is ready for review.

Brett

On Tue, Jan 21, 2014 at 11:31 PM, vladmihalcea [email protected]:

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 [email protected]

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;
public boolean isDisabled();
public void setDisabled(boolean disabled);

And Metrics don't support those, since it would be beyond of the lib
scope.
In my opinion, I think Bitronix should offer JMX register/unregister of
any MBean, be it a Metric or an Operation.
So, BTM should have such service, probably by refactoring the current
ManagementRegistrar codebase.
Some of the current MBeans, displaying
static data (min size, max size) or attributes (disbaled) or operations
(reset) are useful.
Metrics like inPoolSize, totalPoolSize, waitTime are better represented
using the Codahale Metrics adaptor.
For BTM all of those are just MBean that it register and unregister as
needed.

Vlad

On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad [email protected]

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 [email protected]

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 [email protected]

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.
But that's easy knowing that you don't mind changing the naming
convention.

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 [email protected]
wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is
the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 <
[email protected]>

wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation,
rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but
I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]

wrote:

I find it perfectly fair to have to add an extra dep to get any kind
of
monitoring support, and that's exactly what I had in mind: no extra
dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's
   related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics
   API,
   that service just needs to be a thin wrapper around Metrics - there's
   no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow
running
BTM
without the Metrics lib, not create any kind of abstraction. As such,
you
have to stay careful to keep very small methods so that the JIT is be
able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea <
[email protected]>wrote:

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 <
[email protected]>

wrote:

Vlad,

release() cannot be called concurrently: connections taken out of
the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?)
the
JMX monitoring stuff has a poorly designed concurrency access.
That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea <
[email protected]>wrote:

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 <
[email protected]>

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because
it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing
the
value at one time. volatile merely ensures cross-thread
visibility.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32789034>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32889451>
.

�
Reply to this email directly or view it on GitHub.

�
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32892263
.

from btm.

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 [email protected] wrote:

When working with other projects, I usually just do a git rebase -i HEAD~x
in my own cloned repo to squash them in the way I want, then push them,
then submit the pull request. If you've already pushed them to your own
repo, as I tend to do, it is OK. Just squash them locally then do a git
push --force to your repo and it will rewrite history. If you need any
help, just ask.

Brett

On Wed, Jan 22, 2014 at 12:02 AM, vladmihalcea [email protected]:

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 <
[email protected]> wrote:

A pull request is fine. It is preferred if you squash your working commits
into a single commit when you think it is ready for review.

Brett

On Tue, Jan 21, 2014 at 11:31 PM, vladmihalcea [email protected]:

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 [email protected]

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;
public boolean isDisabled();
public void setDisabled(boolean disabled);

And Metrics don't support those, since it would be beyond of the lib
scope.
In my opinion, I think Bitronix should offer JMX register/unregister of
any MBean, be it a Metric or an Operation.
So, BTM should have such service, probably by refactoring the current
ManagementRegistrar codebase.
Some of the current MBeans, displaying
static data (min size, max size) or attributes (disbaled) or operations
(reset) are useful.
Metrics like inPoolSize, totalPoolSize, waitTime are better represented
using the Codahale Metrics adaptor.
For BTM all of those are just MBean that it register and unregister as
needed.

Vlad

On Tuesday, January 21, 2014 5:20 AM, Mihalcea Vlad [email protected]

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 [email protected]

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 [email protected]

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.
But that's easy knowing that you don't mind changing the naming
convention.

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 [email protected]
wrote:

I wouldn't bother about maintaining any kind of backward compatibility
about JMX names, even between minor releases. JMX has never been a
supported API of any kind by BTM.

I've seen this simple example in the doc:

final JmxReporter reporter =
JmxReporter.forRegistry(registry).build();reporter.start();

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 [email protected]:

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);
mBeanServer.registerMBean(new JmxHistogram(histogram, objectName),
objectName);

where:

private ObjectName createName(String type, String name) {
try {
return new ObjectName(this.name, "name", name);
} catch (MalformedObjectNameException e) {
try {
return new ObjectName(this.name, "name",
ObjectName.quote(name));
} catch (MalformedObjectNameException e1) {
LOGGER.warn("Unable to register {} {}", type, name,
e1);
throw new RuntimeException(e1);
}
}
}

So the "this.name" is the domain part, while the "name" variable is
the
current resource name. This gives us to option:

1. split resources by domain: jdbcconnections, jdbcdatasource,
   jdbctransaction, etc
2. split the sub-resource by name: jdbconnection.timers.waitime, where
   timer is the default "type", and the name is waittime.

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 <
[email protected]>

wrote:

Hi,

I am glad you are open to refactror the whole JMX implementation,
rather
than patching it with new features.
This is the right thing to do and I am going to do my best to properly
design it.
I have never designed for making sure JIT always inlines my code, but
I
think this is going to be a great experience.
I will post my finding throughout my development journey.

Keep in tough.

Vlad

On Monday, January 20, 2014 12:22 PM, lorban [email protected]

wrote:

I find it perfectly fair to have to add an extra dep to get any kind
of
monitoring support, and that's exactly what I had in mind: no extra
dep,
no
monitoring.

Here's my mental plan of what I would do:

1. delete the ManagementRegistrar and absolutely everything that's
   related
   to management
2. introduce a management service in the TransactionManagerServices
3. have two implementations of that service: one no-op, one wrapping
   Metrics
4. give that service a very thin API that would mimick the Metrics
   API,
   that service just needs to be a thin wrapper around Metrics - there's
   no
   need to support anything else.

As Brett pointed out, the only goal of that service is to allow
running
BTM
without the Metrics lib, not create any kind of abstraction. As such,
you
have to stay careful to keep very small methods so that the JIT is be
able
to inline them and avoid creating any garbage to prevent any extra GC
work.

On Mon, Jan 20, 2014 at 10:54 AM, vladmihalcea <
[email protected]>wrote:

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 <
[email protected]>

wrote:

Vlad,

release() cannot be called concurrently: connections taken out of
the
pool
are not thread-safe.

I nevertheless agree that this is rather messy. OTOH, mostly (only?)
the
JMX monitoring stuff has a poorly designed concurrency access.
That's
yet
another reason why I'd like to get rid of it all and replace it with
Metrics.

Ludovic

On Mon, Jan 20, 2014 at 6:02 AM, vladmihalcea <
[email protected]>wrote:

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 <
[email protected]>

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 <
[email protected]> 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.
The same is true of ResourceBean.createdResourcesCounter. Because
it
is
only called from the JdbcPooledConnection constructor, and the
constructor
is only called from XAPool.grow() which is synchronized on the
poolGrowthShrinkLock, there will never be two threads incrementing
the
value at one time. volatile merely ensures cross-thread
visibility.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32734381>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32746297>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32789034>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-32889451>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-32892263
.

—
Reply to this email directly or view it on GitHub.

from btm.

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I would
definitely get the free app SourceTree. Once installed you can easily add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to your
github clone. Then in the github UI, in your repo, go to 'Pull requests'
and click on 'New pull request'. This should offer up all of your changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to send
a pull request, squash all of the commits (see below) into one, and send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea [email protected]:

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

from btm.

Hi,

I tried to pull the request but it added the commit to the one I'd already sent you for AtomicInteger.
So it's not an isolated "pull request". If it's messed up, probably it's easier to simply delete it and start it from scratch on a new branch.
Let me know if it's feasible to review it in the current form, or should I try to rollback my changes and try it from scratch.

Vlad

On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge [email protected] wrote:

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I would
definitely get the free app SourceTree. Once installed you can easily add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to your
github clone. Then in the github UI, in your repo, go to 'Pull requests'
and click on 'New pull request'. This should offer up all of your changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to send
a pull request, squash all of the commits (see below) into one, and send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea [email protected]:

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
—
Reply to this email directly or view it on GitHub.

from btm.

It looks reviewable in that form. After any changes as a result of the
review, you can reverse the AtomicInteger commit, and we'll have you squash
them before the pull request.

It's midnight here in Tokyo, but I'll take a look at the changes in the
morning.

Thanks,
Brett

On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea [email protected]:

Hi,

I tried to pull the request but it added the commit to the one I'd already
sent you for AtomicInteger.
So it's not an isolated "pull request". If it's messed up, probably it's
easier to simply delete it and start it from scratch on a new branch.
Let me know if it's feasible to review it in the current form, or should I
try to rollback my changes and try it from scratch.

Vlad

On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge <
[email protected]> wrote:

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I would
definitely get the free app SourceTree. Once installed you can easily add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to your
github clone. Then in the github UI, in your repo, go to 'Pull requests'
and click on 'New pull request'. This should offer up all of your changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to send
a pull request, squash all of the commits (see below) into one, and send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea <[email protected]

wrote:

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
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-33027686
.

from btm.

Hi,

I decided to rollback those 2 commits and only send a pull request with the metrics.
You'll find it in the morning ;)

Vlad

On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge [email protected] wrote:

It looks reviewable in that form. After any changes as a result of the
review, you can reverse the AtomicInteger commit, and we'll have you squash
them before the pull request.

It's midnight here in Tokyo, but I'll take a look at the changes in the
morning.

Thanks,
Brett

On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea [email protected]:

Hi,

I tried to pull the request but it added the commit to the one I'd already
sent you for AtomicInteger.
So it's not an isolated "pull request". If it's messed up, probably it's
easier to simply delete it and start it from scratch on a new branch.
Let me know if it's feasible to review it in the current form, or should I
try to rollback my changes and try it from scratch.

Vlad

On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge <
[email protected]> wrote:

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I would
definitely get the free app SourceTree. Once installed you can easily add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to your
github clone. Then in the github UI, in your repo, go to 'Pull requests'
and click on 'New pull request'. This should offer up all of your changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to send
a pull request, squash all of the commits (see below) into one, and send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea <[email protected]

wrote:

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
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-33027686
.

—
Reply to this email directly or view it on GitHub.

from btm.

Hi,

I rolled-back the AtomicInteger thing, but seems like GitHub still keeps the revered commits in the pull-request list.
Anyhow, this last pull request should contain all changes for Metrics, so it may be easier to review.

Vlad

On Wednesday, January 22, 2014 4:49 PM, Mihalcea Vlad [email protected] wrote:

Hi,

I decided to rollback those 2 commits and only send a pull request with the metrics.
You'll find it in the morning ;)

Vlad

On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge [email protected] wrote:

It looks reviewable in that form. After any changes as a result of the
review, you can reverse the AtomicInteger commit, and we'll have you squash
them before the pull request.

It's midnight here in Tokyo, but I'll take a look at the changes in the
morning.

Thanks,
Brett

On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea [email protected]:

Hi,

I tried to pull the request but it added the commit to the one I'd already
sent you for AtomicInteger.
So it's not an isolated "pull request". If it's messed up, probably it's
easier to simply delete it and start it from scratch on a new branch.
Let me know if it's feasible to review it in the current form, or should I
try to rollback my changes and try it from scratch.

Vlad

On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge <
[email protected]> wrote:

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I would
definitely get the free app SourceTree. Once installed you can easily add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to your
github clone. Then in the github UI, in your repo, go to 'Pull requests'
and click on 'New pull request'. This should offer up all of your changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to send
a pull request, squash all of the commits (see below) into one, and send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea <[email protected]

wrote:

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
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-33027686
.

—
Reply to this email directly or view it on GitHub.

from btm.

Hi,

I see that travis failed on jdk7, but succeeded on jdk6. That's strange, since I tested it with java7, after changing
my JAVA_HOME and PATH env vars to JDK 1.7, and ran:

mvn -P java7 clean install

Can you replicate the failed test?

Vlad

On Wednesday, January 22, 2014 5:03 PM, Mihalcea Vlad [email protected] wrote:

Hi,

I rolled-back the AtomicInteger thing, but seems like GitHub still keeps the revered commits in the pull-request list.
Anyhow, this last pull request should contain all changes for Metrics, so it may be easier to review.

Vlad

On Wednesday, January 22, 2014 4:49 PM, Mihalcea Vlad [email protected] wrote:

Hi,

I decided to rollback those 2 commits and only send a pull request with the metrics.
You'll find it in the morning ;)

Vlad

On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge [email protected] wrote:

It looks reviewable in that form. After any changes as a result of the
review, you can reverse the AtomicInteger commit, and we'll have you squash
them before the pull request.

It's midnight here in Tokyo, but I'll take a look at the changes in the
morning.

Thanks,
Brett

On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea [email protected]:

Hi,

I tried to pull the request but it added the commit to the one I'd already
sent you for AtomicInteger.
So it's not an isolated "pull request". If it's messed up, probably it's
easier to simply delete it and start it from scratch on a new branch.
Let me know if it's feasible to review it in the current form, or should I
try to rollback my changes and try it from scratch.

Vlad

On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge <
[email protected]> wrote:

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I would
definitely get the free app SourceTree. Once installed you can easily add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to your
github clone. Then in the github UI, in your repo, go to 'Pull requests'
and click on 'New pull request'. This should offer up all of your changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to send
a pull request, squash all of the commits (see below) into one, and send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea <[email protected]

wrote:

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
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-33027686
.

—
Reply to this email directly or view it on GitHub.

from btm.

The build does not fail on my computer. I re-ran the TravisCI build and it
succeeded this time. I think we still have a shutdown timing issue in that
test. I will investigate, but don't worry about it for now.

On Thu, Jan 23, 2014 at 8:06 AM, Brett Wooldridge <
[email protected]> wrote:

On the way into the office, I'll try it when I get in. We were seeing
transient failures like that a few months back. See the commit 57db882 on
Nov 22. It was meant to address that issue, and we thought it had.
Anyway, I'll take a look...

On Thu, Jan 23, 2014 at 6:25 AM, vladmihalcea [email protected]:

Hi,

I see that travis failed on jdk7, but succeeded on jdk6. That's strange,
since I tested it with java7, after changing
my JAVA_HOME and PATH env vars to JDK 1.7, and ran:

mvn -P java7 clean install

Can you replicate the failed test?

Vlad

On Wednesday, January 22, 2014 5:03 PM, Mihalcea Vlad [email protected]
wrote:

Hi,

I rolled-back the AtomicInteger thing, but seems like GitHub still keeps
the revered commits in the pull-request list.
Anyhow, this last pull request should contain all changes for Metrics, so
it may be easier to review.

Vlad

On Wednesday, January 22, 2014 4:49 PM, Mihalcea Vlad [email protected]
wrote:

Hi,

I decided to rollback those 2 commits and only send a pull request with
the metrics.
You'll find it in the morning ;)

Vlad

On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge <
[email protected]> wrote:

It looks reviewable in that form. After any changes as a result of the
review, you can reverse the AtomicInteger commit, and we'll have you
squash
them before the pull request.

It's midnight here in Tokyo, but I'll take a look at the changes in the
morning.

Thanks,
Brett

On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea [email protected]:

Hi,

I tried to pull the request but it added the commit to the one I'd
already
sent you for AtomicInteger.
So it's not an isolated "pull request". If it's messed up, probably
it's
easier to simply delete it and start it from scratch on a new branch.
Let me know if it's feasible to review it in the current form, or
should I
try to rollback my changes and try it from scratch.

Vlad

On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge <
[email protected]> wrote:

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I
would
definitely get the free app SourceTree. Once installed you can easily
add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to
your
github clone. Then in the github UI, in your repo, go to 'Pull
requests'
and click on 'New pull request'. This should offer up all of your
changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to
send
a pull request, squash all of the commits (see below) into one, and
send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other
project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea <[email protected]

wrote:

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
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-33027686>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-33069970
.

from btm.

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 [email protected] wrote:

The build does not fail on my computer. I re-ran the TravisCI build and it
succeeded this time. I think we still have a shutdown timing issue in that
test. I will investigate, but don't worry about it for now.

On Thu, Jan 23, 2014 at 8:06 AM, Brett Wooldridge <
[email protected]> wrote:

On the way into the office, I'll try it when I get in. We were seeing
transient failures like that a few months back. See the commit 57db882 on
Nov 22. It was meant to address that issue, and we thought it had.
Anyway, I'll take a look...

On Thu, Jan 23, 2014 at 6:25 AM, vladmihalcea [email protected]:

Hi,

I see that travis failed on jdk7, but succeeded on jdk6. That's strange,
since I tested it with java7, after changing
my JAVA_HOME and PATH env vars to JDK 1.7, and ran:

mvn -P java7 clean install

Can you replicate the failed test?

Vlad

On Wednesday, January 22, 2014 5:03 PM, Mihalcea Vlad [email protected]
wrote:

Hi,

I rolled-back the AtomicInteger thing, but seems like GitHub still keeps
the revered commits in the pull-request list.
Anyhow, this last pull request should contain all changes for Metrics, so
it may be easier to review.

Vlad

On Wednesday, January 22, 2014 4:49 PM, Mihalcea Vlad [email protected]
wrote:

Hi,

I decided to rollback those 2 commits and only send a pull request with
the metrics.
You'll find it in the morning ;)

Vlad

On Wednesday, January 22, 2014 4:41 PM, Brett Wooldridge <
[email protected]> wrote:

It looks reviewable in that form. After any changes as a result of the
review, you can reverse the AtomicInteger commit, and we'll have you
squash
them before the pull request.

It's midnight here in Tokyo, but I'll take a look at the changes in the
morning.

Thanks,
Brett

On Wed, Jan 22, 2014 at 11:32 PM, vladmihalcea [email protected]:

Hi,

I tried to pull the request but it added the commit to the one I'd
already
sent you for AtomicInteger.
So it's not an isolated "pull request". If it's messed up, probably
it's
easier to simply delete it and start it from scratch on a new branch.
Let me know if it's feasible to review it in the current form, or
should I
try to rollback my changes and try it from scratch.

Vlad

On Wednesday, January 22, 2014 4:02 PM, Brett Wooldridge <
[email protected]> wrote:

What OS are you using?

A but unrelated but ... first thing I would do, if Windows or Mac, I
would
definitely get the free app SourceTree. Once installed you can easily
add
your local repo. It looks something like this:

[image: Inline image 1]

If you haven't committed anything, even locally, it's pretty easy. Just
commit them all in one single commit to your local repo. Push up to
your
github clone. Then in the github UI, in your repo, go to 'Pull
requests'
and click on 'New pull request'. This should offer up all of your
changes
(commits) as a pull request.

In the future, I would recommend creating a branch (sometimes called a
feature branch), make all of your commits there. When you're ready to
send
a pull request, squash all of the commits (see below) into one, and
send a
pull request from that in the github UI.

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
anything... but useful for the future and when working with other
project
owners.

On Wed, Jan 22, 2014 at 9:01 PM, vladmihalcea <[email protected]

wrote:

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
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub<
https://github.com/bitronix/btm/issues/7#issuecomment-33027686>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7#issuecomment-33069970
.

—
Reply to this email directly or view it on GitHub.

from btm.