Skip to content

Conversation

@swamirishi
Copy link
Contributor

@swamirishi swamirishi commented Feb 24, 2023

What changes were proposed in this pull request?

JNI for RocksDB SST Dump tool for reading tombstone entries from SST file for smart computation of SnapDiff.
rocks-native-build can be triggered by passing -Drocks_tools_native while maven build.

mvn clean install -DskipTests -Drocks_tools_native -pl :hdds-rocks-native

What is the link to the Apache JIRA

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

How was this patch tested?

Manual Testing.

@swamirishi swamirishi changed the title HDDS-8028: JNI for RocksDB SST Dump tool HDDS-8028. JNI for RocksDB SST Dump tool Feb 24, 2023
@adoroszlai adoroszlai added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Feb 24, 2023
@kerneltime
Copy link
Contributor

kerneltime commented Feb 27, 2023

Open questions

  • How can we avoid building the JNI as part of each build? Can we separate out the JNI build as an independent artifact that gets pulled in as a JNI dependency?
  • Can we build the JNI and update the docker images used via CI/CD to avoid the hit per build

@swamirishi swamirishi marked this pull request as draft March 1, 2023 18:09
@swamirishi swamirishi marked this pull request as ready for review March 2, 2023 00:24
@swamirishi swamirishi closed this Mar 2, 2023
@swamirishi swamirishi reopened this Mar 2, 2023
@swamirishi swamirishi marked this pull request as draft March 2, 2023 17:26
@swamirishi swamirishi marked this pull request as ready for review March 2, 2023 17:26
@neils-dev
Copy link
Contributor

Thanks @swamirishi . If would be a great help if you can let me know if there are any special build parameters you have in mind for building this like the native library compile for hadoop?
ie. $ mvn package -Pdist,native -DskipTests -Dtar

Also, what would be a good way that I can manually test the jni build?

@swamirishi
Copy link
Contributor Author

n let me know if there are any special build parameters you have in mind for building this like the native library compile for hadoop?
There are no special build parameters. Currently the module is built by default for all runs.

@prashantpogde
Copy link
Contributor

Open questions

  • How can we avoid building the JNI as part of each build? Can we separate out the JNI build as an independent artifact that gets pulled in as a JNI dependency?
  • Can we build the JNI and update the docker images used via CI/CD to avoid the hit per build

We are planning to have a seprate profile to enable efficient snapdiff build. The default build will just include regular snapdiff path.

Copy link
Contributor

@prashantpogde prashantpogde left a comment

Choose a reason for hiding this comment

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

Changes look good to me once these pending comments are taken care of. We should write some integration test but I guess we can make it end to end driven by snapdiff.

@smengcl
Copy link
Contributor

smengcl commented Mar 10, 2023

lgtm apart from a few minor comments inline.

Copy link
Contributor

@prashantpogde prashantpogde left a comment

Choose a reason for hiding this comment

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

LGTM

@smengcl
Copy link
Contributor

smengcl commented Mar 10, 2023

Verified that build time is unchanged with normal dev workflow (does not compile RocksDB Tools without -Drocks_tools_native).

Current master branch b1695e3 done in 1m58s with skip tests/javadoc/shade:

$ mvn clean install -DskipTests -e -Dmaven.javadoc.skip=true -DskipShade
...
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:58 min
[INFO] Finished at: 2023-03-10T12:30:59-08:00

Compilation passed locally in 1m59s on db3300d with the same maven args:

$ mvn clean install -DskipTests -e -Dmaven.javadoc.skip=true -DskipShade
...
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:59 min
[INFO] Finished at: 2023-03-10T12:25:36-08:00

With -Drocks_tools_native, compilation passed locally in 3m26s on db3300d with skip tests/javadoc/shade:

$ mvn clean install -DskipTests -e -Dmaven.javadoc.skip=true -DskipShade -Drocks_tools_native
...
[INFO] Apache Ozone HDDS RocksDB Tools .................... SUCCESS [01:27 min]
...
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  03:26 min
[INFO] Finished at: 2023-03-10T14:05:34-08:00

@adoroszlai adoroszlai dismissed their stale review March 10, 2023 20:55

comments addressed

@smengcl smengcl merged commit 354d3dc into apache:master Mar 10, 2023
@smengcl
Copy link
Contributor

smengcl commented Mar 10, 2023

Thanks @swamirishi for the patch. It is a huge effort to put this all together.

Thanks @adoroszlai @prashantpogde for the code review.

if there are any special build parameters you have in mind for building this like the native library compile for hadoop? ie. $ mvn package -Pdist,native -DskipTests -Dtar

Hi @neils-dev , the extra maven parameter has been included in the PR description now, namely -Drocks_tools_native.

@kerneltime w.r.t. to the build time concern, as tested above, by default it will not impact the build time. Adding -Drocks_tools_native to trigger the RocksDB tool compilation adds another ~1.5 min to the total build time (M1 Pro MBP), which is not required for most other dev work.

errose28 added a commit to errose28/ozone that referenced this pull request Mar 16, 2023
* master: (262 commits)
  HDDS-8153. Integrate ContainerBalancer with MoveManager (apache#4391)
  HDDS-8090. When getBlock from a datanode fails, retry other datanodes. (apache#4357)
  HDDS-8163 Use try-with-resources to ensure close rockdb connection in SstFilteringService (apache#4402)
  HDDS-8065. Provide GNU long options (apache#4394)
  HDDS-7930. [addendum] input stream does not refresh expired block token.
  HDDS-7930. input stream does not refresh expired block token. (apache#4378)
  HDDS-7740. [Snapshot] Implement SnapshotDeletingService (apache#4244)
  HDDS-8076. Use container cache in Key listing API. (apache#4346)
  HDDS-8091. [addendum] Generate list of config tags from ConfigTag enum - Hadoop 3.1 compatibility fix (apache#4374)
  HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod fails as we approach DST. (apache#4382)
  HDDS-8151. Support fine grained lifetime for root CA certificate (apache#4386)
  HDDS-8150. RpcClientTest and ConfigurationSourceTest not run due to naming convention (apache#4388)
  HDDS-8131. Add Configuration for OM Ratis Log Purge Tuning Parameters. (apache#4371)
  HDDS-8133. Create ozone sh key checksum command (apache#4375)
  HDDS-8142. Check if no entries in Block DB for a container on container delete (apache#4379)
  HDDS-8118. Fail container delete on non empty chunks dir (apache#4367)
  HDDS-8028. JNI for RocksDB SST Dump tool (apache#4315)
  HDDS-8129. ContainerStateMachine allows two different tasks with the same container id running in parallel. (apache#4370)
  HDDS-8119. Remove loosely related AutoCloseable from SendContainerOutputStream (apache#4368)
  close db connection (apache#4366)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

snapshot https://issues.apache.org/jira/browse/HDDS-6517

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants