Skip to content

HDDS-3833. Use Pipeline choose policy to choose pipeline from exist pipeline list #1096

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
Merged
elek merged 12 commits into from
Aug 11, 2020
Merged

HDDS-3833. Use Pipeline choose policy to choose pipeline from exist pipeline list #1096

elek merged 12 commits into from
Aug 11, 2020

Conversation

@maobaolong
Copy link
Member

@maobaolong maobaolong commented Jun 19, 2020

What changes were proposed in this pull request?

With this policy driven mode, we can develop various pipeline choosing policy to satisfy complex production environment.

Now the default policy is Random policy.

What is the link to the Apache JIRA

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

How was this patch tested?

CI checks.

@maobaolong
Copy link
Member Author

maobaolong commented Jun 19, 2020

@nandakumar131 Would you like to take a look at this PR? Thanks.

@maobaolong maobaolong force-pushed the choosePipelinePolicy branch 3 times, most recently from 087cefa to 370cbb0 Compare June 19, 2020 14:02
@maobaolong
Copy link
Member Author

maobaolong commented Jun 29, 2020

Hi @elek Would like to take a look at this PR?

@maobaolong
Copy link
Member Author

maobaolong commented Jul 17, 2020

@xiaoyuyao Please take a look at this PR, thanks.

avijayanhwx
avijayanhwx reviewed Jul 23, 2020
View reviewed changes
Copy link
Contributor

@avijayanhwx avijayanhwx 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 working on this @maobaolong. The approach looks good to me. I have added a few comments. Also, can we add unit test(s) for this feature?

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/PipelineChoosePolicy.java Outdated
Copy link
Contributor

@avijayanhwx avijayanhwx Jul 23, 2020

Choose a reason for hiding this comment

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

@maobaolong For my knowledge, can you list a few pipeline selection policies that you envision? I am wondering if we are OK with just a List<Pipeline> as an argument.

Copy link
Member Author

@maobaolong maobaolong Jul 24, 2020

Choose a reason for hiding this comment

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

@avijayanhwx Thank you for your suggestion, I add a arguments map, for extension purpose, i use Object type as value type.

@maobaolong maobaolong force-pushed the choosePipelinePolicy branch from 8dfadd9 to 3596e41 Compare July 24, 2020 05:01
@maobaolong
Copy link
Member Author

maobaolong commented Jul 27, 2020

@avijayanhwx Thanks for your pervious review and suggestion, I push a new commit and addressed you comments, PTAL.

elek
elek requested changes Jul 27, 2020
View reviewed changes
Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

Thanks the patch @maobaolong. Overall it looks good to me. I have a few comments inside about some technical details.

hadoop-hdds/common/src/main/resources/ozone-default.xml Outdated
value.
</description>
</property>
<property>
Copy link
Member

@elek elek Jul 27, 2020

Choose a reason for hiding this comment

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

I would prefer to use the new, Java based configuration API:

https://cwiki.apache.org/confluence/display/HADOOP/Java-based+configuration+API

hadoop-hdds/interface-server/src/main/resources/proto.lock Outdated
{
"name": "INTERNAL_ERROR",
"integer": 29
},
Copy link
Member

@elek elek Jul 27, 2020

Choose a reason for hiding this comment

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

Proto.lock file change should be excluded. the proto.lock should represent and old state: the last released state to use it as a base for comparison.

Rebase to the latest master and you won't see the changed proto lock files (which makes it easy to add lock files accidentally)

@avijayanhwx
Copy link
Contributor

avijayanhwx commented Jul 30, 2020

@maobaolong There are still some unaddressed comments here.

  • Using Java based config API
  • Adding unit test(s).
    PTAL

@maobaolong maobaolong force-pushed the choosePipelinePolicy branch from 1cb6a2e to 399d574 Compare July 30, 2020 23:39
@maobaolong
Copy link
Member Author

maobaolong commented Jul 31, 2020

@avijayanhwx @elek I've addressed all your comments and add some ut, PTAL.

avijayanhwx
avijayanhwx approved these changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@avijayanhwx avijayanhwx 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 changes @maobaolong.

@maobaolong maobaolong requested a review from elek August 11, 2020 08:11
elek
elek approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@elek elek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

+1 thanks the update

@elek elek merged commit a79dfae into apache:master Aug 11, 2020
@maobaolong
Copy link
Member Author

maobaolong commented Aug 11, 2020

@elek Thanks you very much for merge this PR, base this framework, we can create various pipeline choose policies.

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)
  ...
rakeshadr pushed a commit to rakeshadr/hadoop-ozone that referenced this pull request Sep 3, 2020
@maobaolong @rakeshadr
HDDS-3833. Use Pipeline choose policy to choose pipeline from exist p…
88b5bc5 
…ipeline list (apache#1096)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elek elek elek approved these changes

@avijayanhwx avijayanhwx avijayanhwx 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 @avijayanhwx @elek