Skip to content

HDDS-3979. Make bufferSize configurable for stream copy - #1212

Merged
elek merged 6 commits into
apache:masterfrom
maobaolong:s3g
Aug 11, 2020
Merged

HDDS-3979. Make bufferSize configurable for stream copy#1212
elek merged 6 commits into
apache:masterfrom
maobaolong:s3g

Conversation

@maobaolong

@maobaolongmaobaolong commented Jul 17, 2020•
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

  • Refactor the code related copy large by using seek to enforce skip method of KeyInputStream and S3WrapperInputStream, the related codes.
    • We can use IOUtils#copyLarge to clarify and reduce the duplicated code.
    • After this, It's easy to specify a buffer by using IOUtils#copyLarge.
  • Make bufferSize configurable for stream copy.
    • So that user like s3g can specify a buffer size to balance the memory and performance.

What is the link to the Apache JIRA

HDDS-3979

How was this patch tested?

Using awscli or goofys

runzhiwang
runzhiwang reviewed Jul 22, 2020
View reviewed changes
Comment threadhadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyInputStream.java Outdated
@maobaolongmaobaolong changed the title HDDS-3979. Use chunkSize as bufferSize for stream copyHDDS-3979. Make bufferSize configurable for stream copyJul 22, 2020
@maobaolong

maobaolong commented Jul 22, 2020

Copy link
Copy Markdown
MemberAuthor

@runzhiwang Thanks for the review, addressed all of your suggestion, PTAL.

runzhiwang
runzhiwang reviewed Jul 22, 2020
View reviewed changes
Comment threadhadoop-hdds/common/src/main/resources/ozone-default.xml
runzhiwang
runzhiwang approved these changes Jul 22, 2020
View reviewed changes

@runzhiwangrunzhiwang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@maobaolong

maobaolong commented Jul 22, 2020

Copy link
Copy Markdown
MemberAuthor

@runzhiwang Thank you for your review and LGTM. @bharatviswa504 Please take a look at this PR, thanks.

elek
elek requested changes Jul 29, 2020
View reviewed changes

@elekelek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks the patch @maobaolong (and the review @runzhiwang).

It LGTM. Based on my understanding we switch to use a normal seek + IOUtils instead of the custom method. (:+1:)

I added a few comments (inline). The only blocker is to add s3g to the config (but I might miss something...)

Comment threadhadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
Comment threadhadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
}

@PostConstruct
public void init() {

@elekelekJul 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

NIT: It can be slightly better to use

 @Inject
private OzoneConfiguration ozoneConfiguration;

And you don't need to expose getClient() in the EndpointBase.

(Didn't try, but it should work, IMHO).

@maobaolongmaobaolongJul 30, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thank you for let me know this high technology

@maobaolong

maobaolong commented Jul 30, 2020

Copy link
Copy Markdown
MemberAuthor

@elek Thanks you for your review and suggestion, i've addressed your comments, PTAL.

elek
elek approved these changes Aug 11, 2020
View reviewed changes

@elekelek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, thanks the update @maobaolong

@elek
elek merged commit 9a702e5 into apache:masterAug 11, 2020
errose28 added a commit to errose28/ozone that referenced this pull request Aug 12, 2020
@errose28
Merge branch 'master' into HDDS-3869
2de4e70
* master: (28 commits)
HDDS-4037. Incorrect container numberOfKeys and usedBytes in SCM after key deletion (apache#1295)
HDDS-3232. Include the byteman scripts in the distribution tar file (apache#1309)
HDDS-4095. Byteman script to debug HCFS performance (apache#1311)
HDDS-4057. Failed acceptance test missing from bundle (apache#1283)
HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete (apache#1286)
HDDS-4061. Pending delete blocks are not always included in #BLOCKCOUNT metadata (apache#1288)
HDDS-4067. Implement toString for OMTransactionInfo (apache#1300)
HDDS-3878. Make OMHA serviceID optional if one (but only one) is defined in the config (apache#1149)
HDDS-3833. Use Pipeline choose policy to choose pipeline from exist pipeline list (apache#1096)
HDDS-3979. Make bufferSize configurable for stream copy (apache#1212)
HDDS-4048. Show more information while SCM version info mismatch (apache#1278)
HDDS-4078. Use HDDS InterfaceAudience/Stability annotations (apache#1302)
HDDS-4034. Add Unit Test for HadoopNestedDirGenerator. (apache#1266)
HDDS-4076. Translate CSI.md into Chinese (apache#1299)
HDDS-4046. Extensible subcommands for CLI applications (apache#1276)
HDDS-4051. Remove whitelist/blacklist terminology from Ozone (apache#1306)
HDDS-4055. Cleanup GitHub workflow (apache#1282)
HDDS-4042. Update documentation for the GA release (apache#1269)
HDDS-4066. Add core-site.xml to intellij configuration (apache#1292)
HDDS-4073. Remove leftover robot.robot (apache#1297)
...
@maobaolongmaobaolong mentioned this pull request Aug 26, 2020
Merged
rakeshadr pushed a commit to rakeshadr/hadoop-ozone that referenced this pull request Sep 3, 2020
@maobaolong@rakeshadr
HDDS-3979. Make bufferSize configurable for stream copy (apache#1212)
86573a4
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elekelekelek approved these changes
@runzhiwangrunzhiwangrunzhiwang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maobaolong@elek@runzhiwang