Skip to content

Conversation

@szetszwo
Copy link
Contributor

What changes were proposed in this pull request?

jmh-generator-annprocess is GPL2.0. It is incompatible with Apache License 2.0. It is currently only used by genesis (a micro-benchmark tool).

We may have to rewrite the tool or move it out from the main Ozone code base.

What is the link to the Apache JIRA

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

How was this patch tested?

NA

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @szetszwo for the patch.

Further references which I think should be removed:

genesis)
ARTIFACT_LIB_DIR="${OZONE_HOME}/share/ozone/lib/ozone-tools"
mkdir -p "$ARTIFACT_LIB_DIR"
if [[ ! -f "$ARTIFACT_LIB_DIR/jmh-core-1.23.jar" ]]; then
echo "jmh-core jar is missing from $ARTIFACT_LIB_DIR, trying to download from maven central (License: GPL + classpath exception)"
curl -o "$ARTIFACT_LIB_DIR/jmh-core-1.23.jar" https://repo1.maven.org/maven2/org/openjdk/jmh/jmh-core/1.23/jmh-core-1.23.jar
fi
if [[ ! -f "$ARTIFACT_LIB_DIR/jopt-simple-4.6.jar" ]]; then
echo "jopt jar is missing from $ARTIFACT_LIB_DIR, trying to download from maven central (License: MIT License)"
curl -o "$ARTIFACT_LIB_DIR/jopt-simple-4.6.jar" https://repo1.maven.org/maven2/net/sf/jopt-simple/jopt-simple/4.6/jopt-simple-4.6.jar
fi
OZONE_CLASSNAME=org.apache.hadoop.ozone.genesis.Genesis
OZONE_RUN_ARTIFACT_NAME="ozone-tools"
;;

ozone_add_subcommand "genesis" client "runs a collection of ozone benchmarks to help with tuning."

<Match>
<Package name="org.apache.hadoop.ozone.genesis.generated" />
</Match>

find target/coverage-classes -name genesis -type d | xargs rm -rf

CONTRIBUTING.md
23:      * Performance: We have multiple type of load generator / benchmark tools (`ozone freon`, `ozone genesis`), which can be used to test cluster and report problems.

hadoop-hdds/docs/content/tools/_index.md
65:   * **genesis**  - Developer Only, Ozone micro-benchmark application.

hadoop-hdds/docs/content/tools/_index.zh.md
60:   * **genesis**  - Ozone 的 benchmark 应用,仅供开发者使用。

@szetszwo
Copy link
Contributor Author

@adoroszlai , thanks a lot for reviewing this.

Suppose genesis is a useful tool. We probably need to move the genesis code to a different maven module or even a different repo. By default, genesis won't be included with Ozone release. It will be available somewhere. Then, we may download it on demand. This is similar to the jmh-core jar in the ozone script.

if [[ ! -f "$ARTIFACT_LIB_DIR/jmh-core-1.23.jar" ]]; then
echo "jmh-core jar is missing from $ARTIFACT_LIB_DIR, trying to download from maven central (License: GPL + classpath exception)"
curl -o "$ARTIFACT_LIB_DIR/jmh-core-1.23.jar" https://repo1.maven.org/maven2/org/openjdk/jmh/jmh-core/1.23/jmh-core-1.23.jar
fi

The current genesis doc and scripts may be still useful. So, how about we fix them separately?

@adoroszlai
Copy link
Contributor

Suppose genesis is a useful tool. We probably need to move the genesis code to a different maven module or even a different repo. By default, genesis won't be included with Ozone release. It will be available somewhere. Then, we may download it on demand. This is similar to the jmh-core jar in the ozone script.

I think we should move Genesis to a separate repo with different license. (I'm not sure if Apache allows such license, nor about the mechanics of a license change.)

Separate project is also recommended by JMH, too: "The recommended way to run a JMH benchmark is to use Maven to setup a standalone project that depends on the jar files of your application."

The current genesis doc and scripts may be still useful. So, how about we fix them separately?

ozone genesis won't work because Genesis.java is being removed. I think leaving the script around would be confusing. I'm OK with fixing it separately, though. Please let me know if I should work on that part.

@szetszwo
Copy link
Contributor Author

szetszwo commented Jan 19, 2022

ozone genesis won't work because Genesis.java is being removed.

It could possibly work:

  1. Suppose genesis.jar, which contains Genesis.class and other classes, is available in maven or somewhere else.
  2. Use curl to download the jar in the ozone script.
curl -o "$ARTIFACT_LIB_DIR/genesis.jar"  https://repo1.maven.org/maven2/org/apache/ozone/.../genesis.jar
  1. Then, Genesis.class is available locally. It can be executed as usual.

Of course, I am open to use other ways running it.

@szetszwo
Copy link
Contributor Author

... Please let me know if I should work on that part.

@adoroszlai , It would be great if you are interested in working on that!

@adoroszlai
Copy link
Contributor

ozone genesis won't work because Genesis.java is being removed.

It could possibly work:

  1. Suppose genesis.jar, which contains Genesis.class and other classes, is available in maven or somewhere else.

Got it, but my point is it will be broken until we create this separate repo and make it available.

@szetszwo
Copy link
Contributor Author

@adoroszlai , Okay, just have removed the genesis scripts and docs.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @szetszwo for updating the patch.

@adoroszlai adoroszlai merged commit 431ce39 into apache:master Jan 20, 2022
errose28 added a commit to errose28/ozone that referenced this pull request Feb 4, 2022
* master: (27 commits)
  HDDS-6206. Application errors must not flood system log (apache#3001)
  HDDS-6181. Change SCMHAInvocationHandler#invokeRatis() logging to TRACE (apache#2992)
  HDDS-5529. For any IOexception from @REPLicated method we should throw it (apache#2788)
  HDDS-6201. Fix NPE for DataScanner with scanned container deleted by others. (apache#3005)
  HDDS-6239. ozonesecure-mr failing with No URLs in mirrorlist (apache#3029)
  HDDS-6227. Test helpers should observe naming conditions (apache#3020)
  HDDS-6205. Add CLI command to display the latest Replication Manager report (apache#3013)
  HDDS-6192. feature/Observability.md translated to Chinese (apache#2994)
  HDDS-6219. Switch to RATIS ReplicationType from STAND_ALONE in our tests. (apache#3014)
  HDDS-6211. [Docs] Image styling on deployed site does not replicate local builds. (apache#3007)
  HDDS-6177. Extend container info command to include replica details  (apache#2995)
  HDDS-6128. CLI tool that downloads all the block replicas and creates a manifest file (apache#2987)
  HDDS-6191. Intermittent failure in TestDeleteWithSlowFollower (apache#3015)
  HDDS-6216. Move OMOpenKeysDeleteRequest to package om.request.key (apache#3011)
  HDDS-6203. CleanUp incomplete gz files during Container move (apache#3000)
  HDDS-3408. Rename ChunkLayOutVersion to ContainerLayoutVersion. (apache#2983)
  HDDS-6109. Preserve the underlying exception raised in client lib. (apache#2989)
  HDDS-6135. SCM Container DB bootstrap on Recon startup for SCM HA. (apache#2972)
  HDDS-6202. Avoid using jmh-generator-annprocess since it is GPL2.0. (apache#2998)
  HDDS-6163. Fix PATH in docker image (apache#2967)
  ...

Conflicts:
    hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java
errose28 added a commit to errose28/ozone that referenced this pull request Feb 7, 2022
* master: (27 commits)
  HDDS-6206. Application errors must not flood system log (apache#3001)
  HDDS-6181. Change SCMHAInvocationHandler#invokeRatis() logging to TRACE (apache#2992)
  HDDS-5529. For any IOexception from @REPLicated method we should throw it (apache#2788)
  HDDS-6201. Fix NPE for DataScanner with scanned container deleted by others. (apache#3005)
  HDDS-6239. ozonesecure-mr failing with No URLs in mirrorlist (apache#3029)
  HDDS-6227. Test helpers should observe naming conditions (apache#3020)
  HDDS-6205. Add CLI command to display the latest Replication Manager report (apache#3013)
  HDDS-6192. feature/Observability.md translated to Chinese (apache#2994)
  HDDS-6219. Switch to RATIS ReplicationType from STAND_ALONE in our tests. (apache#3014)
  HDDS-6211. [Docs] Image styling on deployed site does not replicate local builds. (apache#3007)
  HDDS-6177. Extend container info command to include replica details  (apache#2995)
  HDDS-6128. CLI tool that downloads all the block replicas and creates a manifest file (apache#2987)
  HDDS-6191. Intermittent failure in TestDeleteWithSlowFollower (apache#3015)
  HDDS-6216. Move OMOpenKeysDeleteRequest to package om.request.key (apache#3011)
  HDDS-6203. CleanUp incomplete gz files during Container move (apache#3000)
  HDDS-3408. Rename ChunkLayOutVersion to ContainerLayoutVersion. (apache#2983)
  HDDS-6109. Preserve the underlying exception raised in client lib. (apache#2989)
  HDDS-6135. SCM Container DB bootstrap on Recon startup for SCM HA. (apache#2972)
  HDDS-6202. Avoid using jmh-generator-annprocess since it is GPL2.0. (apache#2998)
  HDDS-6163. Fix PATH in docker image (apache#2967)
  ...
arp7 pushed a commit that referenced this pull request Feb 17, 2022
…ansport support (#3074)

* HDDS-6149. Remove unused keytabs (#2960)

* HDDS-6094. Some unit tests are skipped due to using JUnit4 runner (#2909)

* HDDS-6075. OzoneConfiguration constructor overrides input configuration keys. (#2921)

* HDDS-4177. SCM Container DB bootstrap on Recon startup (#2942)

* HDDS-6086. Compute MD5MD5CRC file checksum using chunk checksums from DataNodes (#2919)

* HDDS-6148. Validate ContainerBalancerConfiguration before start ContainerBalancer (#2957)

* HDDS-6161. SCM StateMachine failing to reinitialize doesn't terminate the process. (#2971)

* HDDS-6134. Move replication-specific config to ReplicationServer (#2943)

* HDDS-4010. S3G startup fails when multiple service ids are configured. (#2976)

* HDDS-6170. Add metrics to replication manager to track container health states (#2975)

* HDDS-3231. Cleanup KeyManagerImpl (#2961)

* HDDS-5927. Improve defaults in ContainerBalancerConfiguration (#2892)

* HDDS-6157. More consistent synchronization in InputStreams (#2965)

* HDDS-4743. [FSO] Add FSO variant of ITestOzoneContractDistcp. (#2980)

* HDDS-6114. Intermittent error due to Failed to init RocksDB (#2947)

* HDDS-6175. Use s3Auth during proxy during decrypt in RpcClient. (#2981)

* HDDS-6175. Use s3Auth during proxy during decrypt in RpcClient.

* HDDS-5740. Enable ratis by default for SCM. (#2637)

* HDDS-6183. Intermittent failure in TestKeyDeletingService.checkIfDeleteServiceWithFailingSCM. (#2991)

* HDDS-4190. Intermittent failure in TestOMVolumeSetOwnerRequest and TestOMVolumeSetQuotaRequest. (#2982)

* HDDS-6120. Compute block checksum using chunk checksums (#2930)

* HDDS-6147. Add ability in OM to get limited delta updates (#2956)

* HDDS-6195. Remove unused jmh-core dependency. (#2997)

* HDDS-6167. Update ozone-runner version to 20211202-1 (#2969)

* HDDS-6171. Create an API to change Bucket Owner (#2988)

* HDDS-6163. Fix PATH in docker image (#2967)

* HDDS-6202. Avoid using jmh-generator-annprocess since it is GPL2.0. (#2998)

* HDDS-6135. SCM Container DB bootstrap on Recon startup for SCM HA. (#2972)

* HDDS-6109. Preserve the underlying exception raised in client lib. (#2989)

* HDDS-3408. Rename ChunkLayOutVersion to ContainerLayoutVersion. (#2983)

* HDDS-6203. CleanUp incomplete gz files during Container move (#3000)

* HDDS-6216. Move OMOpenKeysDeleteRequest to package om.request.key (#3011)

* HDDS-6191. Intermittent failure in TestDeleteWithSlowFollower (#3015)

* HDDS-6128. CLI tool that downloads all the block replicas and creates a manifest file (#2987)

* HDDS-6177. Extend container info command to include replica details  (#2995)

* HDDS-6211. [Docs] Image styling on deployed site does not replicate local builds. (#3007)

* HDDS-6219. Switch to RATIS ReplicationType from STAND_ALONE in our tests. (#3014)

* HDDS-6192. feature/Observability.md translated to Chinese (#2994)

* HDDS-6205. Add CLI command to display the latest Replication Manager report (#3013)

* HDDS-6227. Test helpers should observe naming conditions (#3020)

* HDDS-6239. ozonesecure-mr failing with No URLs in mirrorlist (#3029)

* HDDS-6201. Fix NPE for DataScanner with scanned container deleted by others. (#3005)

* HDDS-5529. For any IOexception from @REPLicated method we should throw it (#2788)

* HDDS-6181. Change SCMHAInvocationHandler#invokeRatis() logging to TRACE (#2992)

* HDDS-6206. Application errors must not flood system log (#3001)

* HDDS-6245. Add BucketLayout logging to Audit Logs (#3040)

* HDDS-6238 Reduce memory requirements for list keys. (#3032)

* HDDS-2919. Intermittent failure in TestRDBStore (#3028)

* HDDS-6253. Unnecessary duplicate smoketest after defaulting to FSO (#3036)

* HDDS-6204. Cleanup handling malformed authorization header (#2999)

* HDDS-6169. Selective checks: skip junit tests on ozone-runner image update (#2974)

* HDDS-6270. Use a dedicated file instead of /etc/passwd for xcompat acceptance test (#3050)

* HDDS-6273. Amend doc SecuringTDE.md (#3047)

* HDDS-6140. Selective checks: skip unit check for integration-test changes (#2948)

* HDDS-6215. Recon get limited delta updates from OM (#3009)

* HDDS-6125. Recon get limited delta updates from OM

* HDDS-6215. Fix unit test

* trigger new CI check

* HDDS-6215. Fix typo

* trigger new CI check

Co-authored-by: Symious <[email protected]>

* HDDS-6226. Run tests for selective CI checks in CI (#3019)

* HDDS-6247. Avoid logging stack trace for user input problems (#3039)

* HDDS-6208. New checkstyle: WhitespaceAround (#3003)

* HDDS-6289. Upgrade acceptance test log flooded with parse error (#3063)

Co-authored-by: Siyao Meng <[email protected]>

* Trigger Build

* Fix integration test for added configuation field for selecting OmTransport for s3 gateway - TestOzoneConfigurationFields (added config key not in xml).

Co-authored-by: Doroszlai, Attila <[email protected]>
Co-authored-by: Lokesh Jain <[email protected]>
Co-authored-by: Aswin Shakil Balasubramanian <[email protected]>
Co-authored-by: Wei-Chiu Chuang <[email protected]>
Co-authored-by: Symious <[email protected]>
Co-authored-by: Bharat Viswanadham <[email protected]>
Co-authored-by: Stephen O'Donnell <[email protected]>
Co-authored-by: GeorgeJahad <[email protected]>
Co-authored-by: Siddhant Sangwan <[email protected]>
Co-authored-by: Jyotinder Singh <[email protected]>
Co-authored-by: Shashikant Banerjee <[email protected]>
Co-authored-by: Tsz-Wo Nicholas Sze <[email protected]>
Co-authored-by: Kiyoshi Mizumaru <[email protected]>
Co-authored-by: Ritesh H Shukla <[email protected]>
Co-authored-by: Nibiru <[email protected]>
Co-authored-by: Kaijie Chen <[email protected]>
Co-authored-by: Zita Dombi <[email protected]>
Co-authored-by: Istvan Fajth <[email protected]>
Co-authored-by: steinsgateted <[email protected]>
Co-authored-by: Gui Hecheng <[email protected]>
Co-authored-by: Jackson Yao <[email protected]>
Co-authored-by: Keyi Song <[email protected]>
Co-authored-by: UENISHI Kota <[email protected]>
Co-authored-by: Siyao Meng <[email protected]>
Co-authored-by: Symious <[email protected]>
@sodonnel
Copy link
Contributor

For the record, I strongly disagree with how this was handled. Useful code was just removed here because you could not be bothered to setup something else to house the code. I don't recall seeing anything on a mailing list about this.

I get that is should not have been committed in the first place, but it was, and as one of the people who contributed to these benchmarks I feel quit insulted it was just thrown away.

I have now went looking for code to benchmark a change to an area one of these covered and have seen quite a bit of inconvenience trying to track it down.

@sodonnel
Copy link
Contributor

I am also inclined to say this change should be reverted and handled correctly, moving the code elsewhere and notifying the community of its location and a strategy for adding such benchmarks in the future.

@szetszwo
Copy link
Contributor Author

... Useful code was just removed here ...

@sodonnel , The code may be useful but it violates Apache policy; see https://www.apache.org/legal/resolved.html#category-x . We cannot include it in any Apache projects. Sorry!

I get that is should not have been committed in the first place, ...

Usually, we will reopen the JIRAs and revert the commits. However, there were multiple commits done long time ago. Reverting them becomes impossible or very hard. In such case, we usually remove the code in a separated JIRA. Please don't take it personal.

@szetszwo
Copy link
Contributor Author

I am also inclined to say this change should be reverted and handled correctly, moving the code elsewhere and notifying the community of its location and a strategy for adding such benchmarks in the future.

I am totally fine if we want to revert this pull request. No hard feelings at all.

@adoroszlai
Copy link
Contributor

@sodonnel I apologize for removing the benchmarks from this repo. I have filtered the commits related to Genesis to a separate repo. It's a personal one for now, but we can work on moving it to apache.

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