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.

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

HDDS-2720. Ozone Failure injection Service #956

Merged
merged 10 commits into from
Jun 8, 2020

Conversation

prashantpogde
Copy link
Contributor

What changes were proposed in this pull request?

Adding Fault Injection Service in Ozone:tools/FaultInjectionService directory

What is the link to the Apache JIRA

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

How was this patch tested?

This is an independent tool which will be used to do fault injection testing for Ozone. Its build/test process is independent of Ozone and should not impact Ozone in any way.
For using this tool, please follow README in tools/FaultInjectionService.

@elek elek changed the title Hdds 2720 HDDS-2720. Ozone Failure injection Service May 22, 2020
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.

Thank you very much to open this issue. I am very happy that it was decided to move to the umbrella of Ozone.

I had some minor comments, and also two generic one:

  1. Directory name: Can you please use full lower case dir names (FaultInjectionService --> fault-injection-service). I am your friend in using linux, but AFAIK the upper-case/lower-case dirs can cause problems on other OS like OSX.

  2. Build script: If we have no build script to check it, it can become obsolete over the time. Ideally we need to test it during the builds.

I would be happy to help to include it to the github actions, but can you please provide a very simple script to build this service?. I would suggest to create a hadoop-ozone/dev-support/checks/buildfaultinjection.sh.

Use can use the existing pattern to identify the dir:

  DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
  cd "$DIR/../../.." || exit 1

And add your 1-2 commands (./configure, ./make whatever)

@@ -0,0 +1,12 @@
Current Maintainer
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we need this file (but I can be convinced). Until now we didn't follow this pattern. There are no maintainers of SCM / Freon / OM. The community itself is the maintainers but we can check the git history for first contact.

This file is also requires additional maintenance. I would suggest to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elek Thank you for reviewing it.

  • Yes I will remove AUTHORS file and change the directory name to lower case.
  • Its supported only on linux and some dependencies require building them from the source. I will add an automated build script as next set of enhancement and create a separate PR.


About
------
TBD
Copy link
Member

Choose a reason for hiding this comment

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

Just one sentence please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, will change.


Development Status
------------------
TBD
Copy link
Member

Choose a reason for hiding this comment

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

One word, please ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, will change.

@elek
Copy link
Member

elek commented May 28, 2020

Thanks the update @prashantpogde

  1. Can you please remove tools/fault-injection-service/LICENSE? As I wrote this is unnecessary as the whole project is distributed under the top level LICENSE file.

  2. License header check doesn't work for this dir (as I see). Can you please double check if ASF headers are added to everywhere? Seems to be missing from the Test*.cc files

  3. I am fine to create the build script in the next PR. After 1-2, I will merge this.

@prashantpogde
Copy link
Contributor Author

@elek Thank you for the review. I have made the suggested changes. Please take a look.

@@ -0,0 +1,56 @@
Fault Injection Service
Copy link
Member

Choose a reason for hiding this comment

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

ASF licence header is missing from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that ASF license header was not there in Ozone README.md, Should I put it in this one ?

@elek
Copy link
Member

elek commented Jun 8, 2020

I just found this one on the faq page

Other files may make sense to have no license header. Three examples are: ... Short informational text files; for example README, INSTALL files

It seems that we don't need LICENSE header for that specific README.

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.

+1 Thanks the work @prashantpogde

Will merge it soon.

@elek elek merged commit 3328d7d into apache:master Jun 8, 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.

2 participants