Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move com.hazelcast.monitor to internal package #15888

Merged
merged 2 commits into from Oct 31, 2019

Conversation

@mmedenjak
Copy link
Contributor

mmedenjak commented Oct 29, 2019

EE: hazelcast/hazelcast-enterprise#3292

Most probably the very last of the big public API cleanup PRs. This one focuses only on statistics. Requested everyone according to their area of expertise.

Most of these are only used in:

  • diagnostics (which is private API)
  • JMX (only specific methods are invoked, not entire interfaces are
    exposed)
  • JSON for member-to-MC communication (which is private API)
    This makes most interfaces actually private API. We leave some that are
    exposed directly on other public API.

Moved to private API:
@blazember @emre-aydin
com.hazelcast.monitor.HotRestartState
com.hazelcast.monitor.LocalCacheStats
com.hazelcast.monitor.LocalFlakeIdGeneratorStats
com.hazelcast.monitor.LocalGCStats
com.hazelcast.monitor.LocalMemoryStats
com.hazelcast.monitor.LocalOperationStats
com.hazelcast.monitor.LocalPNCounterStats
com.hazelcast.monitor.LocalRecordStoreStats
com.hazelcast.monitor.LocalWanPublisherStats
com.hazelcast.monitor.LocalWanStats
com.hazelcast.monitor.MemberPartitionState
com.hazelcast.monitor.MemberState
com.hazelcast.monitor.NodeState
com.hazelcast.monitor.WanSyncState
+the entire com.hazelcast.monitor.impl package

Kept in public API but moved to other packages:
@blazember
com.hazelcast.monitor.LocalQueueStats
com.hazelcast.monitor.LocalExecutorStats
com.hazelcast.monitor.LocalInstanceStats
com.hazelcast.monitor.JsonSerializable
com.hazelcast.monitor.LocalMapStats
com.hazelcast.monitor.LocalMultiMapStats
com.hazelcast.monitor.NearCacheStats cc @ahmetmircik
com.hazelcast.monitor.LocalReplicatedMapStats
com.hazelcast.monitor.LocalTopicStats

Also the usual checkstyle cleanup.

@mmedenjak mmedenjak added this to the 4.0 milestone Oct 29, 2019
@mmedenjak mmedenjak requested a review from hazelcast/clients as a code owner Oct 29, 2019
@mmedenjak mmedenjak self-assigned this Oct 29, 2019
@@ -15,10 +15,10 @@
*/


package com.hazelcast.monitor;
package com.hazelcast.internal.monitor;

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Oct 29, 2019

Author Contributor

WAN stats are currently exposed as public SPI but will be moved into private SPI in a subsequent PR.

@@ -67,7 +65,7 @@
* lastStatisticsCollectionTime: The time stamp (milliseconds since epoch) when the latest update for the statistics is
* collected.
*
* Near cache statistics (see {@link com.hazelcast.monitor.NearCacheStats}):
* Near cache statistics (see {@link NearCacheStats}):

This comment has been minimized.

Copy link
@blazember

blazember Oct 29, 2019

Contributor

I believe this needs a change in the client protocol definition too, otherwise, the next generation will override it.

This comment has been minimized.

Copy link
@blazember

blazember Oct 29, 2019

Contributor

The protocol definition is this: https://github.com/hazelcast/hazelcast-client-protocol/blob/master/protocol-definitions/Client.yaml#L647
No need to do it now, I need to touch this codec anyways with the metric work. A note for myself.

Matko Medenjak added 2 commits Oct 29, 2019
Most of these are only used in:
- diagnostics (which is private API)
- JMX (only specific methods are invoked, not entire interfaces are
exposed)
- JSON for member-to-MC communication (which is private API)
This makes most interfaces actually private API. We leave some that are
exposed directly on other public API.

Moved to private API:
com.hazelcast.monitor.HotRestartState
com.hazelcast.monitor.LocalCacheStats
com.hazelcast.monitor.LocalFlakeIdGeneratorStats
com.hazelcast.monitor.LocalGCStats
com.hazelcast.monitor.LocalMemoryStats
com.hazelcast.monitor.LocalOperationStats
com.hazelcast.monitor.LocalPNCounterStats
com.hazelcast.monitor.LocalRecordStoreStats
com.hazelcast.monitor.LocalWanPublisherStats
com.hazelcast.monitor.LocalWanStats
com.hazelcast.monitor.MemberPartitionState
com.hazelcast.monitor.MemberState
com.hazelcast.monitor.NodeState
com.hazelcast.monitor.WanSyncState
+the entire com.hazelcast.monitor.impl package

Kept in public API but moved to other packages:
com.hazelcast.monitor.LocalQueueStats
com.hazelcast.monitor.LocalExecutorStats
com.hazelcast.monitor.LocalInstanceStats
com.hazelcast.monitor.JsonSerializable
com.hazelcast.monitor.LocalMapStats
com.hazelcast.monitor.LocalMultiMapStats
com.hazelcast.monitor.NearCacheStats
com.hazelcast.monitor.LocalReplicatedMapStats
com.hazelcast.monitor.LocalTopicStats

Also the usual checkstyle cleanup.
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-private-api-cleanup-8 branch from e3da3f4 to 5dcc9e3 Oct 30, 2019
@ahmetmircik

This comment has been minimized.

Copy link
Member

ahmetmircik commented Oct 31, 2019

@mmedenjak Could you please remind me what was the reasoning behind having LocalMapStats under package com.hazelcast.monitor? Since, for 4.0, we have moved all data-structures to its own package, for instance, moved all IMap related public api under com.hazelcast.map. But now LocalMapStats seems an exception given that it is under monitoring package.

@sancar
sancar approved these changes Oct 31, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Oct 31, 2019

@ahmetmircik LocalMapStats was under com.hazelcast.monitor (probably because we assumed it was only used in member-MC communication) but now it's under com.hazelcast.map. Can you check again?
https://github.com/hazelcast/hazelcast/pull/15888/files#diff-f79afb70914a40c63aea9fb491205c70

@ahmetmircik

This comment has been minimized.

Copy link
Member

ahmetmircik commented Oct 31, 2019

@mmedenjak it was me wrong, thanks. Fully makes sense now.

@mmedenjak mmedenjak merged commit 7a594ee into hazelcast:master Oct 31, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@mmedenjak mmedenjak deleted the mmedenjak:4.0-private-api-cleanup-8 branch Oct 31, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Oct 31, 2019

Thank you for the reviews everyone!

mmedenjak pushed a commit to mmedenjak/hazelcast that referenced this pull request Nov 15, 2019
JsonSerializable is supposed to be used only when exchanging statistics
with management center. Previously, it was public API because some
statistics classes such as LocalMapStats were exposed as public methods.

Now we move JsonSerializable to private API. Because of this, we have to
cast whenever we want to transform from/to JSON as most internal code
exchanges stats interfaces such as LocalMapStats. The alternative is
that the internal API exchanges concrete classes such as
LocalMapStatsImpl but this is not possible since sometimes there are two
different implementations for the same stats interface.

Addresses hazelcast#15888 (comment)
mmedenjak pushed a commit to mmedenjak/hazelcast that referenced this pull request Nov 18, 2019
JsonSerializable is supposed to be used only when exchanging statistics
with management center. Previously, it was public API because some
statistics classes such as LocalMapStats were exposed as public methods.

Now we move JsonSerializable to private API. Because of this, we have to
cast whenever we want to transform from/to JSON as most internal code
exchanges stats interfaces such as LocalMapStats. The alternative is
that the internal API exchanges concrete classes such as
LocalMapStatsImpl but this is not possible since sometimes there are two
different implementations for the same stats interface.

Addresses hazelcast#15888 (comment)
mmedenjak added a commit that referenced this pull request Nov 18, 2019
Move JsonSerializable to private API

JsonSerializable is supposed to be used only when exchanging statistics
with management center. Previously, it was public API because some
statistics classes such as LocalMapStats were exposed as public methods.

Now we move JsonSerializable to private API. Because of this, we have to
cast whenever we want to transform from/to JSON as most internal code
exchanges stats interfaces such as LocalMapStats. The alternative is
that the internal API exchanges concrete classes such as
LocalMapStatsImpl but this is not possible since sometimes there are two
different implementations for the same stats interface.

Addresses #15888 (comment)
petrpleshachkov pushed a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 22, 2019
Move JsonSerializable to private API

JsonSerializable is supposed to be used only when exchanging statistics
with management center. Previously, it was public API because some
statistics classes such as LocalMapStats were exposed as public methods.

Now we move JsonSerializable to private API. Because of this, we have to
cast whenever we want to transform from/to JSON as most internal code
exchanges stats interfaces such as LocalMapStats. The alternative is
that the internal API exchanges concrete classes such as
LocalMapStatsImpl but this is not possible since sometimes there are two
different implementations for the same stats interface.

Addresses hazelcast#15888 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.