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

Deprecate STATISTICS properties #19219

Merged
merged 2 commits into from
Jul 30, 2021

Conversation

sancar
Copy link
Contributor

@sancar sancar commented Jul 29, 2021

The following properties are not used anymore since 4.0

  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);

I traced it back to this pr. It seems that we are checking metricsConfig instead.
#15963

This pr:

  1. puts @Deprecated to the property documents on ClientProperties class.
  2. If user sets statistics, make sure it will set metrics config as well. This is because there
    were no warning or documentation about the removal. According to doc, these properties still used.
    So we fix the behaviour.
  3. If both properties are set, ignore old STATISTICS properties.
  4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes #18579

Checklist:

  • Labels (Team:, Type:, Source:, Module:) and Milestone set
  • Add Add to Release Notes label if changes should be mentioned in release notes or Not Release Notes content if changes are not relevant for release notes
  • Request reviewers if possible
  • New public APIs have @Nonnull/@Nullable annotations
  • New public APIs have @since tags in Javadoc
  • Send backports/forwardports if fix needs to be applied to past/future releases

The following properties are not used anymore since 4.0

```
  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);
```
I traced it back to this pr. It seems that we are checking metricsConfig instead.

This pr:

1. puts @deprecated to the property documents on ClientProperties class.
2. If user sets statistics, make sure it will set metrics config as well. This is because there
were no warning or documentation about the removal. According to doc, these properties still used.
So we fix the behaviour.
3. If both properties are set, ignore old STATISTICS properties.
4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes hazelcast#18579
@sancar sancar added Team: Client Source: Internal PR or issue was opened by an employee Add to Release Notes labels Jul 29, 2021
@sancar sancar requested a review from blazember July 29, 2021 10:13
@sancar sancar self-assigned this Jul 29, 2021
@sancar sancar requested a review from a team as a code owner July 29, 2021 10:13
@sancar sancar added this to the 5.0 milestone Jul 29, 2021
@srknzl
Copy link
Member

srknzl commented Jul 29, 2021

metricsConfig does not exist on Node.js client and statistics properties are used. Should I add metricsConfig, or is it fine to use statistics properties?

Copy link
Contributor

@mdumandag mdumandag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from some Javadoc problems. Can we also add a test for it to verify that this is working as intended?

@sancar sancar merged commit fde1a42 into hazelcast:master Jul 30, 2021
@sancar sancar deleted the fix/statisticsProperties/master branch July 30, 2021 13:02
sancar pushed a commit to sancar/hazelcast that referenced this pull request Jul 30, 2021
* Deprecate STATISTICS properties

The following properties are not used anymore since 4.0

```
  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);
```
I traced it back to this pr. It seems that we are checking metricsConfig instead.

This pr:

1. puts @deprecated to the property documents on ClientProperties class.
2. If user sets statistics, make sure it will set metrics config as well. This is because there
were no warning or documentation about the removal. According to doc, these properties still used.
So we fix the behaviour.
3. If both properties are set, ignore old STATISTICS properties.
4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes hazelcast#18579

* fix javadoc styling and add test to verify new behaviour

(cherry picked from commit fde1a42)
sancar pushed a commit to sancar/hazelcast that referenced this pull request Jul 30, 2021
* Deprecate STATISTICS properties

The following properties are not used anymore since 4.0

```
  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);
```
I traced it back to this pr. It seems that we are checking metricsConfig instead.

This pr:

1. puts @deprecated to the property documents on ClientProperties class.
2. If user sets statistics, make sure it will set metrics config as well. This is because there
were no warning or documentation about the removal. According to doc, these properties still used.
So we fix the behaviour.
3. If both properties are set, ignore old STATISTICS properties.
4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes hazelcast#18579

* fix javadoc styling and add test to verify new behaviour

(cherry picked from commit fde1a42)
sancar pushed a commit to sancar/hazelcast that referenced this pull request Jul 30, 2021
* Deprecate STATISTICS properties

The following properties are not used anymore since 4.0

```
  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);
```
I traced it back to this pr. It seems that we are checking metricsConfig instead.

This pr:

1. puts @deprecated to the property documents on ClientProperties class.
2. If user sets statistics, make sure it will set metrics config as well. This is because there
were no warning or documentation about the removal. According to doc, these properties still used.
So we fix the behaviour.
3. If both properties are set, ignore old STATISTICS properties.
4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes hazelcast#18579

* fix javadoc styling and add test to verify new behaviour

(cherry picked from commit fde1a42)
sancar pushed a commit that referenced this pull request Aug 2, 2021
* Deprecate STATISTICS properties

The following properties are not used anymore since 4.0

```
  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);
```
I traced it back to this pr. It seems that we are checking metricsConfig instead.

This pr:

1. puts @deprecated to the property documents on ClientProperties class.
2. If user sets statistics, make sure it will set metrics config as well. This is because there
were no warning or documentation about the removal. According to doc, these properties still used.
So we fix the behaviour.
3. If both properties are set, ignore old STATISTICS properties.
4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes #18579

* fix javadoc styling and add test to verify new behaviour

(cherry picked from commit fde1a42)
sancar pushed a commit that referenced this pull request Aug 2, 2021
* Deprecate STATISTICS properties

The following properties are not used anymore since 4.0

```
  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);
```
I traced it back to this pr. It seems that we are checking metricsConfig instead.

This pr:

1. puts @deprecated to the property documents on ClientProperties class.
2. If user sets statistics, make sure it will set metrics config as well. This is because there
were no warning or documentation about the removal. According to doc, these properties still used.
So we fix the behaviour.
3. If both properties are set, ignore old STATISTICS properties.
4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes #18579

* fix javadoc styling and add test to verify new behaviour

(cherry picked from commit fde1a42)
sancar pushed a commit that referenced this pull request Aug 2, 2021
* Deprecate STATISTICS properties

The following properties are not used anymore since 4.0

```
  public static final HazelcastProperty STATISTICS_ENABLED = new HazelcastProperty("hazelcast.client.statistics.enabled",
            false);
    /**
     * The period in seconds the statistics run.
     */
    public static final HazelcastProperty STATISTICS_PERIOD_SECONDS = new HazelcastProperty(
            "hazelcast.client.statistics.period.seconds", 3, SECONDS);
```
I traced it back to this pr. It seems that we are checking metricsConfig instead.

This pr:

1. puts @deprecated to the property documents on ClientProperties class.
2. If user sets statistics, make sure it will set metrics config as well. This is because there
were no warning or documentation about the removal. According to doc, these properties still used.
So we fix the behaviour.
3. If both properties are set, ignore old STATISTICS properties.
4. Adds javadoc to ClientProperty and MetricsConfig to explain the effects of using these properties

fixes #18579

* fix javadoc styling and add test to verify new behaviour

(cherry picked from commit fde1a42)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add to Release Notes All Languages Should Check Used by clients team to track fixes on the java client that should potentially backported to others Module: Config Source: Internal PR or issue was opened by an employee Team: Client Type: Defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClientProperties about ClientStatistics is not used
4 participants