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.

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

Remove MathUtils.now() #1837

Merged
merged 2 commits into from
Nov 25, 2018
Merged

Remove MathUtils.now() #1837

merged 2 commits into from
Nov 25, 2018

Conversation

sijie
Copy link
Member

@sijie sijie commented Nov 24, 2018 •
edited
Loading

Descriptions of the changes in this PR:

Motivation

MathUtils.now() using System.nanoTime doesn't provide accurate time. It is used
for measuring pupose. However since nanoTime can have numeric overflow, so when
it is used for converting into milliseconds, it is misleading and error-prone. A
lot of places using MathUtis.now can all replaced with System.currentTimeMillis().

It leads to unknown behavior on compactions. examples is shown in the following figure.

wechatimg180

example: this graph shows the major_compaction_count. major compaction is supposed to run every day. but in the graph, there is no major compaction occuring in 4 days.

Changes

Remove MathUtils.now() and replace it with System.currentTimeMillis().

In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks. However running all
the precommit checks can take a long time, some trivial changes don't need to run all the precommit checks. You
can check following list to skip the tests that don't need to run for your pull request. Leave them unchecked if
you are not sure, committers will help you:

  • [skip bookkeeper-server bookie tests]: skip testing org.apache.bookkeeper.bookie in bookkeeper-server module.
  • [skip bookkeeper-server client tests]: skip testing org.apache.bookkeeper.client in bookkeeper-server module.
  • [skip bookkeeper-server replication tests]: skip testing org.apache.bookkeeper.replication in bookkeeper-server module.
  • [skip bookkeeper-server tls tests]: skip testing org.apache.bookkeeper.tls in bookkeeper-server module.
  • [skip bookkeeper-server remaining tests]: skip testing all other tests in bookkeeper-server module.
  • [skip integration tests]: skip docker based integration tests. if you make java code changes, you shouldn't skip integration tests.
  • [skip build java8]: skip build on java8. ONLY skip this when ONLY changing files under documentation under site.
  • [skip build java9]: skip build on java9. ONLY skip this when ONLY changing files under documentation under site.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue #>: Description of pull request
    e.g. Issue 123: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue #> in the title with the actual Issue number.

@sijie
Remove MathUtils.now()
597e074 
*Motivation*

`MathUtils.now()` using System.nanoTime doesn't provide accurate time. It is used
for measuring pupose. However since `nanoTime` can have numeric overflow, so when
it is used for converting into milliseconds, it is misleading and error-prone. A
lot of places using `MathUtis.now` can all replaced with `System.currentTimeMillis()`.

*Changes*

Remove `MathUtils.now()` and replace it with `System.currentTimeMillis()`.
@sijie sijie added this to the 4.9.0 milestone Nov 24, 2018
@sijie sijie self-assigned this Nov 24, 2018
@sijie
Copy link
Member Author

sijie commented Nov 24, 2018

wechatimg180

example: this graph shows the major_compaction_count. major compaction is supposed to run every day. but in the graph, there is no major compaction occuring in 4 days.

eolivelli
eolivelli requested changes Nov 24, 2018
View reviewed changes
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/MathUtils.java
* @return current time in milliseconds.
*/
public static long now() {
return System.nanoTime() / NANOSECONDS_PER_MILLISECOND;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we call here System.currentTimeMillis and leave the rest unchanged?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is confusing. I would rather use System.currentTimeMillis to be more explicit

eolivelli
eolivelli approved these changes Nov 24, 2018
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Okay.

In other applications/benchmarks I have seen that nanoTime() is not even usable with multithreaded applications: problems arise when you sample the start point with one CPU/core and the second point with another CPU/core.

So I am totally +1 in not using nanoTime for metrics

@sijie
Copy link
Member Author

sijie commented Nov 24, 2018

Enrico, to be clear, I am not removing the usage of metrics. The main change is to remove it being used in scheduling.

@sijie
Copy link
Member Author

sijie commented Nov 25, 2018

run bookkeeper-server replication tests

@sijie sijie merged commit a32e29d into apache:master Nov 25, 2018
sijie added a commit that referenced this pull request Nov 25, 2018
@sijie
Remove MathUtils.now()
2db1b13 
Descriptions of the changes in this PR:

*Motivation*

`MathUtils.now()` using System.nanoTime doesn't provide accurate time. It is used
for measuring pupose. However since `nanoTime` can have numeric overflow, so when
it is used for converting into milliseconds, it is misleading and error-prone. A
lot of places using `MathUtis.now` can all replaced with `System.currentTimeMillis()`.

It leads to unknown behavior on compactions. examples is shown in the following figure.

<img width="1157" alt="wechatimg180" src="https://user-images.githubusercontent.com/1217863/48965961-d860b600-ef7c-11e8-9394-1d51cef5c68c.png">

example: this graph shows the major_compaction_count. major compaction is supposed to run every day. but in the graph, there is no major compaction occuring in 4 days.

*Changes*

Remove `MathUtils.now()` and replace it with `System.currentTimeMillis()`.

Reviewers: Jia Zhai <None>, Enrico Olivelli <eolivelli@gmail.com>

This closes #1837 from sijie/remove_mathutils_now

(cherry picked from commit a32e29d)
Signed-off-by: Sijie Guo <sijie@apache.org>
sijie added a commit that referenced this pull request Nov 25, 2018
@sijie
Remove MathUtils.now()
8189fb0 
Descriptions of the changes in this PR:

*Motivation*

`MathUtils.now()` using System.nanoTime doesn't provide accurate time. It is used
for measuring pupose. However since `nanoTime` can have numeric overflow, so when
it is used for converting into milliseconds, it is misleading and error-prone. A
lot of places using `MathUtis.now` can all replaced with `System.currentTimeMillis()`.

It leads to unknown behavior on compactions. examples is shown in the following figure.

<img width="1157" alt="wechatimg180" src="https://user-images.githubusercontent.com/1217863/48965961-d860b600-ef7c-11e8-9394-1d51cef5c68c.png">

example: this graph shows the major_compaction_count. major compaction is supposed to run every day. but in the graph, there is no major compaction occuring in 4 days.

*Changes*

Remove `MathUtils.now()` and replace it with `System.currentTimeMillis()`.

Reviewers: Jia Zhai <None>, Enrico Olivelli <eolivelli@gmail.com>

This closes #1837 from sijie/remove_mathutils_now

(cherry picked from commit a32e29d)
Signed-off-by: Sijie Guo <sijie@apache.org>
@eolivelli
Copy link
Contributor

@sijie it is clear to me thanks.

My comment was more general, maybe little off topic. nanoTime is meaningful only when used to take timings only on the same thread and without sleeps or waits which cold lead to using a different CPU core

@sijie sijie deleted the remove_mathutils_now branch November 25, 2018 22:40
@sijie sijie mentioned this pull request Nov 26, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@jiazhai jiazhai jiazhai approved these changes

Assignees

@sijie sijie

Labels
area/bookie release/4.7.3 release/4.8.2 release/4.9.0 type/bug
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@sijie @eolivelli @jiazhai