Skip to content

Conversation

@rakeshadr
Copy link
Contributor

What changes were proposed in this pull request?

Refresh pipeline info does a call to SCM and it can be moved outside the BUCKET_LOCK, this would help to improve the performance of read/write mix workloads.

What is the link to the Apache JIRA

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

How was this patch tested?

Using TestOzoneFileSystem unit test cases.

@mukul1987 mukul1987 requested a review from bharatviswa504 July 6, 2020 06:13
Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

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

LGTM, just few minor comments inline...

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

Overall LGTM.
While reading I found one more thing, generating of the secret token can also be moved outside lock.

@codecov-commenter
Copy link

Codecov Report

Merging #1164 into master will decrease coverage by 0.13%.
The diff coverage is 79.59%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1164      +/-   ##
============================================
- Coverage     73.50%   73.36%   -0.14%     
+ Complexity    10031    10025       -6     
============================================
  Files           973      974       +1     
  Lines         49663    49721      +58     
  Branches       4881     4896      +15     
============================================
- Hits          36503    36477      -26     
- Misses        10846    10914      +68     
- Partials       2314     2330      +16     
Impacted Files Coverage Δ Complexity Δ
...ava/org/apache/hadoop/ozone/om/KeyManagerImpl.java 63.96% <79.59%> (-0.38%) 128.00 <7.00> (+3.00) ⬇️
...otocol/commands/RetriableDatanodeEventWatcher.java 55.55% <0.00%> (-44.45%) 3.00% <0.00%> (-1.00%)
...hdds/scm/container/CloseContainerEventHandler.java 72.41% <0.00%> (-17.25%) 6.00% <0.00%> (ø%)
...hdds/scm/container/common/helpers/ExcludeList.java 86.95% <0.00%> (-13.05%) 19.00% <0.00%> (-3.00%)
...ozone/container/ozoneimpl/ContainerController.java 63.15% <0.00%> (-10.53%) 11.00% <0.00%> (-2.00%)
...ache/hadoop/ozone/om/codec/S3SecretValueCodec.java 90.90% <0.00%> (-9.10%) 3.00% <0.00%> (-1.00%)
...e/hadoop/ozone/recon/tasks/OMUpdateEventBatch.java 75.00% <0.00%> (-8.34%) 6.00% <0.00%> (-1.00%)
.../transport/server/ratis/ContainerStateMachine.java 71.52% <0.00%> (-5.39%) 63.00% <0.00%> (-4.00%)
.../common/volume/RoundRobinVolumeChoosingPolicy.java 80.95% <0.00%> (-4.77%) 5.00% <0.00%> (-1.00%)
...apache/hadoop/hdds/server/events/EventWatcher.java 77.77% <0.00%> (-4.17%) 14.00% <0.00%> (ø%)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6eebf2f...3547106. Read the comment docs.

@rakeshadr
Copy link
Contributor Author

@xiaoyuyao @bharatviswa504,
Please review the updated changes when you get a chance. Thanks!

Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @rakeshadr . Latest update LGTM, just two more minor comments.

@rakeshadr
Copy link
Contributor Author

Thanks @xiaoyuyao for the comments. I have updated PR, kindly review it again.

@rakeshadr
Copy link
Contributor Author

Note:- TestWatchForCommit#test2WayCommitForTimeoutException failure is unrelated to my changes. It is passing in my local env.

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@xiaoyuyao xiaoyuyao merged commit 46e7b2f into apache:master Jul 16, 2020
@rakeshadr
Copy link
Contributor Author

Thanks @xiaoyuyao and @bharatviswa504 for the reviews and committing the PR.

errose28 added a commit to errose28/ozone that referenced this pull request Jul 17, 2020
* master:
  HDDS-3855. Add upgrade smoketest (apache#1142)
  HDDS-3964. Ratis config key mismatch (apache#1204)
  HDDS-3612. Allow mounting bucket under other volume (apache#1104)
  HDDS-3926. OM Token Identifier table should use in-house serialization. (apache#1182)
  HDDS-3824: OM read requests should make SCM#refreshPipeline outside BUCKET_LOCK (apache#1164)
errose28 added a commit to errose28/ozone that referenced this pull request Jul 17, 2020
…erface

* upstream/master:
  HDDS-3855. Add upgrade smoketest (apache#1142)
  HDDS-3964. Ratis config key mismatch (apache#1204)
  HDDS-3612. Allow mounting bucket under other volume (apache#1104)
  HDDS-3926. OM Token Identifier table should use in-house serialization. (apache#1182)
  HDDS-3824: OM read requests should make SCM#refreshPipeline outside BUCKET_LOCK (apache#1164)
  HDDS-3966. Disable flaky TestOMRatisSnapshots
errose28 added a commit to errose28/ozone that referenced this pull request Jul 20, 2020
* master:
  HDDS-3984. Support filter and search the columns in recon UI (apache#1218)
  HDDS-3806. Support recognize aws v2 Authorization header. (apache#1098)
  HDDS-3955. Unable to list intermediate paths on keys created using S3G. (apache#1196)
  HDDS-3741. Reload old OM state if Install Snapshot from Leader fails (apache#1129)
  HDDS-3965. SCM failed to start up for duplicated pipeline detected. (apache#1210)
  HDDS-3855. Add upgrade smoketest (apache#1142)
  HDDS-3964. Ratis config key mismatch (apache#1204)
  HDDS-3612. Allow mounting bucket under other volume (apache#1104)
  HDDS-3926. OM Token Identifier table should use in-house serialization. (apache#1182)
  HDDS-3824: OM read requests should make SCM#refreshPipeline outside BUCKET_LOCK (apache#1164)
  HDDS-3966. Disable flaky TestOMRatisSnapshots
errose28 pushed a commit to errose28/ozone that referenced this pull request Jul 21, 2020
ChenSammi pushed a commit that referenced this pull request Jul 22, 2020
rakeshadr added a commit to rakeshadr/hadoop-ozone that referenced this pull request Sep 3, 2020
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.

5 participants