Skip to content

Conversation

@ferhui
Copy link
Contributor

@ferhui ferhui commented Jan 20, 2021

What changes were proposed in this pull request?

Change metrics unit from nanosecond to millisecond uniformly

Some metrics unit is nanosecond, and some is millisecond. The metrics are following

EventWatcherMetrics   ms

ProtocolMessageMetrics ns

CSMMetrics ns

ContainerMetrics ns

XceiverClientMetrics ns

OzoneManagerDoubleBufferMetrics ns

OzoneManagerSyncMetrics ns

ContainerCacheMetrics ms

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4725

How was this patch tested?

manual tests

@ferhui ferhui requested review from ChenSammi and elek January 20, 2021 09:14
@ferhui
Copy link
Contributor Author

ferhui commented Jan 20, 2021

@ChenSammi @elek Could you please take a look?Thanks

@amaliujia
Copy link
Contributor

amaliujia commented Jan 20, 2021

So the reason to convert nano to millis is because that most of the unit are already millis?

Could there be a reason that nano as unit actually could be better?

@ferhui
Copy link
Contributor Author

ferhui commented Jan 21, 2021

So the reason to convert nano to millis is because that most of the unit are already millis?

Could there be a reason that nano as unit actually could be better?

@amaliujia Thanks for comments. As mentioned in JIRA HDDS-4725, I think there are some reasons that it's better to use millis.

  • Mostly the metrics are the order of millisecond.

  • It's millis in hadoop, and It's easier to understand the metric.

  • there are both nano and milli right now, we need to unify them.

@amaliujia
Copy link
Contributor

Thanks @ferhui for you explanation! Let's see if other people have comments on this.

@amaliujia
Copy link
Contributor

+1 LGTM

@ferhui
Copy link
Contributor Author

ferhui commented Jan 22, 2021

@amaliujia Thanks for review! I will merge this if no other comments later.

Copy link
Contributor

@runzhiwang runzhiwang left a comment

Choose a reason for hiding this comment

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

LGTM

@runzhiwang runzhiwang merged commit b83d4b2 into apache:master Jan 22, 2021
@ferhui ferhui deleted the HDDS-4725 branch January 22, 2021 01:57
@ferhui
Copy link
Contributor Author

ferhui commented Jan 22, 2021

@runzhiwang Thanks for review & merge

errose28 added a commit to errose28/ozone that referenced this pull request Feb 1, 2021
* master: (176 commits)
  HDDS-4760. Intermittent failure in ozone-ha acceptance test (apache#1853)
  HDDS-4770. Upgrade Ratis Thirdparty to 0.6.0 (apache#1868)
  HDDS-4765. Update close-pending workflow for new repo (apache#1856)
  HDDS-4737. Add ModifierOrder to checkstyle rules (apache#1839)
  HDDS-4704. Add permission check in OMDBCheckpointServlet (apache#1801)
  HDDS-4757. Unnecessary WARNING to set OZONE_CONF_DIR (apache#1849)
  HDDS-4751. TestOzoneFileSystem#testTrash failed when enabledFileSystemPaths and omRatisDisabled (apache#1851)
  HDDS-4736. Intermittent failure in testExpiredCertificate (apache#1838)
  HDDS-4758. Adjust classpath of ozone version to include log4j (apache#1850)
  HDDS-4518. Add metrics around Trash Operations. (apache#1832)
  HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100) (apache#1805)
  HDDS-4748. sonarqube issue fix - "static" members should be accessed statically (apache#1748)
  HDDS-2402. Adapt hadolint check to improved CI framework (apache#1778)
  HDDS-4698. Upgrade Java for Sonar check (apache#1800)
  HDDS-4739. Upgrade Ratis to 1.1.0-eb66796d-SNAPSHOT (apache#1842)
  HDDS-4735. Fix typo in hdds.proto (apache#1837)
  HDDS-4430. OM failover timeout is too short (apache#1807)
  HDDS-4477. Delete txnId in SCMMetadataStoreImpl may drop to 0 after SCM restart. (apache#1828)
  HDDS-4688. Update Hadoop version to 3.2.2 (apache#1795)
  HDDS-4725. Change metrics unit from nanosecond to millisecond (apache#1823)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants