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.

Sign up for GitHub

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

Jump to bottom

IGNITE-18534 The WalWritingRate metric is not calculated when walMode is LOG_ONLY or BACKGROUND #10508

Merged

nizhikov merged 3 commits into apache:master from luchnikovbsk:IGNITE-18534_master

Mar 1, 2023

Contributor

luchnikovbsk commented Jan 31, 2023 •

edited

Loading

Moved the calculation of the metric to another place, which will work in any WalMode.
The result of running the reproducer before and after fixing in https://issues.apache.org/jira/browse/IGNITE-18534.
A commit with a reproducer is not for merging.

The Contribution Checklist

There is a single JIRA ticket related to the pull request.
The web-link to the pull request is attached to the JIRA ticket.
The JIRA ticket has the Patch Available state.
The pull request body describes changes that have been made.
The description explains WHAT and WHY was made instead of HOW.
The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
A reviewer has been mentioned through the JIRA comments
(see the Maintainers list)
The pull request has been checked by the Teamcity Bot and
the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.

luchnikovbsk force-pushed the IGNITE-18534_master branch from 7363a71 to 4c7c6be Compare

January 31, 2023 11:14

zstan reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                                  .setWalArchivePath(DFLT_WAL_PATH)
+                                  .setWalSegments(10)
+                                  .setWalSegmentSize((int)(1 * MULTIPLICATOR))
+                                  .setMaxWalArchiveSize(20 * MULTIPLICATOR)

Contributor

zstan Jan 31, 2023 •

edited

Loading

20 * MB more clear than your proposal, i suppose.

Contributor Author

luchnikovbsk Jan 31, 2023 •

edited

Loading

Ok, MULTIPLICATOR i use for debug, this is not the final PR, this is a PR for questions.

Is this a good fix?
Write a test that will check the correctness of the calculation of this metric?

The current test is a reproducer.

Contributor Author

luchnikovbsk Feb 27, 2023

done

luchnikovbsk force-pushed the IGNITE-18534_master branch 4 times, most recently from bbab9ed to dd966fa Compare

February 1, 2023 11:53

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                      List<Long> walWritingRates = new ArrayList<>();
+                      long lastSegment = walMgr(ignite).currentSegment() + MAX_SEGMENTS;
+                      long prewSegment = walMgr(ignite).currentSegment();

Contributor

nizhikov Feb 27, 2023

typo: prew -> prev

Contributor Author

luchnikovbsk Feb 27, 2023

done 0_o

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                   */
+                  @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+                      //for debug
+                      LogExporterSpi exporterSpi = new LogExporterSpi();

Contributor

nizhikov Feb 27, 2023

It seems we can remove this.

Contributor Author

luchnikovbsk Feb 27, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  }
+                  /**
+                   * {@inheritDoc}

Contributor

nizhikov Feb 27, 2023

This should be one line comment like in other places.

Contributor Author

luchnikovbsk Feb 27, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  }
+                  /**
+                   * {@inheritDoc}

Contributor

nizhikov Feb 27, 2023

This should be one line comment like in other places.

Contributor Author

luchnikovbsk Feb 27, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  /**
+                   * Small report for debug.
+                   */
+                  private static final Map<String, List<Long>> report = new HashMap<>();

Contributor

nizhikov Feb 27, 2023

It seems this can be removed.

Contributor Author

luchnikovbsk Feb 27, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Show resolved Hide resolved

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  /**
+                   * MULTIPLICATOR.
+                   */
+                  private static final long MULTIPLICATOR = U.MB;

Contributor

nizhikov Feb 27, 2023

You can use U.MB everywhere in the test without additional review.

Contributor Author

luchnikovbsk Feb 27, 2023

deleted it

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  /**
+                   * Тumber of segments.
+                   */
+                  private static final int MAX_SEGMENTS = 20;

Contributor

nizhikov Feb 27, 2023

Do we really need this much segements in the test?

Contributor Author

luchnikovbsk Feb 27, 2023 •

edited

Loading

In the test, the value of the metric is taken after the segment is filled.
Decreased to 1 to speed up test execution.
It seemed more correct to get the value of the metric by filling the segment than by reaching some time, for example, 1 second after the start of recording.

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  /**
+                   * {@inheritDoc}
+                   */
+                  @Override protected void afterTestsStopped() throws Exception {

Contributor

nizhikov Feb 27, 2023

this method can be removed.

Contributor Author

luchnikovbsk Feb 27, 2023

done


          IGNITE-18534 The WalWritingRate metric is not calculated when walMode…

667018a

… is LOG_ONLY or BACKGROUND

luchnikovbsk force-pushed the IGNITE-18534_master branch 2 times, most recently from 8638ceb to 667018a Compare

February 27, 2023 10:05


          Merge branch 'master' into IGNITE-18534_master

luchnikovbsk force-pushed the IGNITE-18534_master branch 4 times, most recently from 8ae221c to baa46c0 Compare

February 27, 2023 11:57

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                              new DataStorageConfiguration()
+                                  .setMetricsEnabled(true)
+                                  .setWalArchivePath(DFLT_WAL_PATH)
+                                  .setWalSegmentSize(1 * 1024 * 1024)

Contributor

nizhikov Feb 28, 2023

1 * 1024 * 1024 -> U.MB

Contributor Author

luchnikovbsk Feb 28, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  /** {@inheritDoc} */
+                  @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+                      return super.getConfiguration(igniteInstanceName)
+                          .setCacheConfiguration(new CacheConfiguration<>(DEFAULT_CACHE_NAME))

Contributor

nizhikov Feb 28, 2023

This can be removed and replaced with ignite.getOrCreateCache

Contributor Author

luchnikovbsk Feb 28, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                      AtomicLong key = new AtomicLong();
+                      List<Long> walWritingRates = new ArrayList<>();
+                      long lastSegment = walMgr(ignite).currentSegment() + 1;

Contributor

nizhikov Feb 28, 2023

Let's extract walMgr(ignite) into variable.

Contributor Author

luchnikovbsk Feb 28, 2023

deleted it

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                      AtomicLong key = new AtomicLong();
+                      List<Long> walWritingRates = new ArrayList<>();
+                      long lastSegment = walMgr(ignite).currentSegment() + 1;

Contributor

nizhikov Feb 28, 2023

It seems we don't need to care about segments here.
Le'ts just do the following:

long k = 0;
for (; k<100; k++) {
    long[] arr = new long[64];
    Arrays.fill(arr, k);

    ignite.getOrCreateCache(DEFAULT_CACHE_NAME).put(key, arr);

    Long writingRate =  metricRegistry(ignite.name(), "io", "datastorage").getAttribute("WalWritingRate");
    if (writingRate != null && writingRate > 0)
        break;
}

assertTrue(k < 100);

Contributor Author

luchnikovbsk Feb 28, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                          .setDataStorageConfiguration(
+                              new DataStorageConfiguration()
+                                  .setMetricsEnabled(true)
+                                  .setWalArchivePath(DFLT_WAL_PATH)

Contributor

nizhikov Feb 28, 2023

Do we really need WalArchivePath for this test?

Contributor Author

luchnikovbsk Feb 28, 2023

no, deleted

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+              public class IoDatastorageMetricsTest extends GridCommonAbstractTest {
+                  /**
+                   * WALMode.

Contributor

nizhikov Feb 28, 2023

This can be oneline comment.

Contributor Author

luchnikovbsk Feb 28, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                  public WALMode walMode;
+                  /**
+                   * WALMode values.

Contributor

nizhikov Feb 28, 2023

This can be oneline comment.

Contributor Author

luchnikovbsk Feb 28, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+              import static org.junit.Assert.assertNotEquals;
+              /**
+               * The test shows that the WalWritingRate metric is not calculated when walMode is in (LOG_ONLY, BACKGROUND).

Contributor

nizhikov Feb 28, 2023

Test checks that WalWritingRate metric updated in all WAL modes.

Contributor Author

luchnikovbsk Feb 28, 2023

done

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Show resolved Hide resolved

luchnikovbsk force-pushed the IGNITE-18534_master branch from bd62026 to b8a2a56 Compare

February 28, 2023 12:49

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated

+                   * The test shows that the WalWritingRate metric is not calculated when walMode in all modes.
+                   */
+                  @Test
+                  public void WalWritingRate() throws Exception {

Contributor

nizhikov Feb 28, 2023

Please, rename WalWritingRate to testWalWritingRate or walWritingRate.

Contributor Author

luchnikovbsk Feb 28, 2023

done

luchnikovbsk force-pushed the IGNITE-18534_master branch from 9562a72 to d81c5bb Compare

February 28, 2023 12:53

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Show resolved Hide resolved

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Show resolved Hide resolved

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Show resolved Hide resolved

nizhikov reviewed

View reviewed changes

...core/src/test/java/org/apache/ignite/internal/processors/cache/IoDatastorageMetricsTest.java Outdated


		ignite.getOrCreateCache(DEFAULT_CACHE_NAME).put(i, arr);

		writingRate = (long)metricRegistry(ignite.name(), "io", "datastorage").getAttribute("WalWritingRate");

Contributor

nizhikov Feb 28, 2023

Empty line after this required.

Contributor Author

luchnikovbsk Feb 28, 2023

done

nizhikov approved these changes

View reviewed changes

luchnikovbsk force-pushed the IGNITE-18534_master branch 3 times, most recently from 5dde17e to f6992ef Compare

February 28, 2023 12:59


          IGNITE-18534 The WalWritingRate metric is not calculated when walMode…

b3f4668

… is LOG_ONLY or BACKGROUND

luchnikovbsk force-pushed the IGNITE-18534_master branch from d9aebaf to b3f4668 Compare

February 28, 2023 13:02

nizhikov merged commit 9db91c4 into apache:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet