HDDS-8489. Refactor GET request to POST for OM DB snapshot #4695

mladjan-gadzic · 2023-05-10T21:55:25Z

What changes were proposed in this pull request?

The om snapshot subsystem will require long arrays of http parameters which will no longer work as url parameters, so we need to change the om bootstrap http request from a http-get to http-post in order to allow form based parameters.

Recon has not yet been modified to support incremental checkpointing and so will not require the long excludeList, so it continues to use http get.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tests
Integration tests
Manual tests

…Snapshot DB checkpoint

…URL parameters

GeorgeJahad · 2023-05-10T22:48:31Z

...manager/src/main/java/org/apache/hadoop/ozone/om/ratis_snapshot/OmRatisSnapshotProvider.java

@@ -76,7 +81,7 @@ public class OmRatisSnapshotProvider extends RDBSnapshotProvider {
  private final Map<String, OMNodeDetails> peerNodesMap;
  private final HttpConfig.Policy httpPolicy;
  private final boolean spnegoEnabled;
-  private final URLConnectionFactory connectionFactory;
+  private URLConnectionFactory connectionFactory;


When possible, I prefer not to remove the final modifier just to make tests work. Could we change the setConnectionFactory() to a getConnectionFactory() method, and mock that method in the test?

Unfortunately I could not find any other way. I need to mock connectionFactory field which is not a part of constructor parameters. I tried using @Mock/@IncjectMock combination, Reflection and Unsafe, but without results. Another idea would be to create constructor specificly for testing purposes, but I do not like that approach.

Do you have an idea how this could be handled?

I just meant something like this: https://github.com/GeorgeJahad/ozone/compare/da4e99768a..urlConnectionFactory

Oh, that is totally a different story. Sure thing!

When possible, I prefer not to remove the final modifier just to make tests work. Could we change the setConnectionFactory() to a getConnectionFactory() method, and mock that method in the test?

+1 on using final.

Don't have very strong opinion but I think it would be nicer to pass URLConnectionFactory as constructor parameter to achieve deterministic behavior for testing. Also aligns with Dependency injection.

Thanks @hemantk-12 for the review! I added constructor which could be used for testing but also alligns with DI.

GeorgeJahad · 2023-05-10T23:08:08Z

...one/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java

+        "Content-Disposition: form-data; name=\"" +
+        OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST + "[]\"" + crNl +
+        crNl +
+        sstFileName + crNl +


please use 2 sst files. (Sometimes testing an array of size 1 hides flaws in loop code.)

GeorgeJahad · 2023-05-10T23:09:18Z

...ger/src/test/java/org/apache/hadoop/ozone/om/ratis_snapshot/TestOmRatisSnapshotProvider.java

+
+    OmRatisSnapshotProvider.writeFormData(connection, sstFiles);
+
+    sb.append(fileName).append(CR_NL);


please use 2 files

GeorgeJahad · 2023-05-10T23:14:11Z

...one/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java

        responseMock);

    doCallRealMethod().when(omDbCheckpointServletMock)
        .writeDbDataToStream(any(), any(), any(), any(), any());
  }

  @Test
-  public void testDoGet() throws Exception {
+  public void testDoPost() throws Exception {


Since we are keeping doGet() around for recon, we need to keep the doGet() test working as well. The test is long and there is not much difference between the two versions, so I would prefer to parameterize this test so it runs twice, once with doGet() and once with doPost(). Is that difficult?

… JUnit4

GeorgeJahad · 2023-05-20T00:07:45Z

hey @Xushaohong these are the doPost() changes I discussed with you. Please take a look.

Xushaohong · 2023-06-06T08:17:07Z

Hi @mladjan-gadzic, thanks for the work on refactoring.
Could you resolve the failing CI first?

mladjan-gadzic · 2023-06-06T08:48:27Z

Hi @mladjan-gadzic, thanks for the work on refactoring. Could you resolve the failing CI first?

Hi @Xushaohong, thanks for the review! I rerun it immediately after it failed and it was a success. Please have a look https://github.com/mladjan-gadzic/ozone/actions/runs/5174385281.

Xushaohong · 2023-06-06T09:47:49Z

Hi @Xushaohong, thanks for the review! I rerun it immediately after it failed and it was a success. Please have a look https://github.com/mladjan-gadzic/ozone/actions/runs/5174385281.

Your link branch is HDDS-7922.
You need to update this branch mladjan-gadzic:HDDS-8489

mladjan-gadzic · 2023-06-06T10:44:20Z

Hi @Xushaohong, thanks for the review! I rerun it immediately after it failed and it was a success. Please have a look https://github.com/mladjan-gadzic/ozone/actions/runs/5174385281.

Your link branch is HDDS-7922. You need to update this branch mladjan-gadzic:HDDS-8489

Mistakenly I pasted wrong url. Please, have a look at https://github.com/mladjan-gadzic/ozone/actions/runs/5066045581.

hemantk-12

Overall looks good to me.
Left couple of minor comments.

hemantk-12 · 2023-05-19T22:20:57Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java

+          continue;
+        }
+
+        if (!item.getFieldName().equals(fieldName)) {


nit: if conditions can be combined to one.

Suggested change

if (!item.getFieldName().equals(fieldName)) {

if (!fieldName.equals(item.getFieldName())

to avoid NPE because we know fieldName is constant.

Conditions are combined and comparation is reversed.

hemantk-12 · 2023-06-06T18:37:42Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java

+   * @return array of parsed sst form data parameters for exclusion
+   */
+  private static String[] parseFormDataParameters(HttpServletRequest request) {
+    String fieldName = OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST + "[]";


This can be a constant.

FIELD_NAME constant is created.

smengcl · 2023-06-08T20:41:23Z

Hi @mladjan-gadzic , would you fix the build failure:

https://github.com/apache/ozone/actions/runs/5066046001/jobs/9137638263?pr=4695

Thx!

mladjan-gadzic · 2023-06-12T09:27:58Z

Hi @mladjan-gadzic , would you fix the build failure:

https://github.com/apache/ozone/actions/runs/5066046001/jobs/9137638263?pr=4695

Thx!

Hi @smengcl, thanks for checking out this PR. I pushed the latest changes and CI succeeded.

# Conflicts: # hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java

mladjan-gadzic · 2023-06-16T11:37:32Z

Hi @smengcl. I fixed missing import, merged upstream/master and resolved conflicts. After all of that remote CI passed.

hemantk-12

LGTM. Thanks for the patch @mladjan-gadzic

smengcl

Thanks @mladjan-gadzic for the patch.

smengcl · 2023-06-20T22:56:32Z

And thanks @GeorgeJahad and @hemantk-12 for reviewing this.

mladjan-gadzic added 6 commits May 9, 2023 10:07

Add POST endpoint and extract common logic from GET endpoint for the …

f3176f3

…Snapshot DB checkpoint

Use POST endpoint on client side and form data parameters instead of …

85c9ddc

…URL parameters

Add tests for server and client side for snapshot download

17ea715

Set charset instead of default one and add package-info

65d920b

Fix charset

6b17abe

Merge remote-tracking branch 'upstream/master' into HDDS-8489

a737f64

GeorgeJahad reviewed May 10, 2023

View reviewed changes

mladjan-gadzic added 4 commits May 11, 2023 14:00

Use 2 sst file names as an example instead of 1

6ffb03c

Parametrize endpoint tests with HTTP method and use JUnit5 instead of…

2d9399c

… JUnit4

Merge remote-tracking branch 'upstream/master' into HDDS-8489

da4e997

Revert removal of final modifier

b3d20d5

neils-dev added gr snapshot https://issues.apache.org/jira/browse/HDDS-6517 labels May 11, 2023

mladjan-gadzic marked this pull request as ready for review May 12, 2023 07:04

Create constructor instead of using a spy

5204f7f

hemantk-12 reviewed Jun 6, 2023

View reviewed changes

Address PR comments

c92835b

mladjan-gadzic added 3 commits June 16, 2023 09:33

Merge remote-tracking branch 'upstream/master' into HDDS-8489

e5bde2e

Fix import

e8e3baa

Merge remote-tracking branch 'upstream/master' into HDDS-8489

50c7ff6

# Conflicts: # hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java

hemantk-12 approved these changes Jun 20, 2023

View reviewed changes

smengcl approved these changes Jun 20, 2023

View reviewed changes

smengcl merged commit 80ba150 into apache:master Jun 20, 2023
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-8489. Refactor GET request to POST for OM DB snapshot #4695

HDDS-8489. Refactor GET request to POST for OM DB snapshot #4695

mladjan-gadzic commented May 10, 2023 •

edited

GeorgeJahad May 10, 2023

mladjan-gadzic May 11, 2023

GeorgeJahad May 11, 2023

mladjan-gadzic May 11, 2023

hemantk-12 May 19, 2023

mladjan-gadzic May 24, 2023

GeorgeJahad May 10, 2023

GeorgeJahad May 10, 2023

GeorgeJahad May 10, 2023

GeorgeJahad commented May 20, 2023 •

edited

Xushaohong commented Jun 6, 2023

mladjan-gadzic commented Jun 6, 2023

Xushaohong commented Jun 6, 2023

mladjan-gadzic commented Jun 6, 2023

hemantk-12 left a comment

hemantk-12 May 19, 2023

mladjan-gadzic Jun 12, 2023

hemantk-12 Jun 6, 2023

mladjan-gadzic Jun 12, 2023

smengcl commented Jun 8, 2023

mladjan-gadzic commented Jun 12, 2023

mladjan-gadzic commented Jun 16, 2023

hemantk-12 left a comment

smengcl left a comment

smengcl commented Jun 20, 2023


		OmRatisSnapshotProvider.writeFormData(connection, sstFiles);

		sb.append(fileName).append(CR_NL);

	if (!item.getFieldName().equals(fieldName)) {
	if (!fieldName.equals(item.getFieldName())

HDDS-8489. Refactor GET request to POST for OM DB snapshot #4695

HDDS-8489. Refactor GET request to POST for OM DB snapshot #4695

Conversation

mladjan-gadzic commented May 10, 2023 • edited

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeJahad commented May 20, 2023 • edited

Xushaohong commented Jun 6, 2023

mladjan-gadzic commented Jun 6, 2023

Xushaohong commented Jun 6, 2023

mladjan-gadzic commented Jun 6, 2023

hemantk-12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smengcl commented Jun 8, 2023

mladjan-gadzic commented Jun 12, 2023

mladjan-gadzic commented Jun 16, 2023

hemantk-12 left a comment

Choose a reason for hiding this comment

smengcl left a comment

Choose a reason for hiding this comment

smengcl commented Jun 20, 2023

mladjan-gadzic commented May 10, 2023 •

edited

GeorgeJahad commented May 20, 2023 •

edited