You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "mladjan-gadzic (via GitHub)" <gi...@apache.org> on 2023/05/10 21:55:27 UTC
[GitHub] [ozone] mladjan-gadzic opened a new pull request, #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
mladjan-gadzic opened a new pull request, #4695:
URL: https://github.com/apache/ozone/pull/4695
## What changes were proposed in this pull request?
POST endpoint is created alongside GET because there are a quite a few Recon test that depend on it. GET endpoint is not a requirement for proper Snapshot bootstraping. It is not used from SCM/OM anymore. Proposal is to create a separate ticket to refactor above-mentioned Recon tests and remove GET endpoint completely.
## 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Xushaohong commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1578156688
Hi @mladjan-gadzic, thanks for the work on refactoring.
Could you resolve the failing CI first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1199472242
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java:
##########
@@ -216,6 +219,68 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) {
}
}
+ /**
+ * Parses request form data parameters.
+ * @param request the HTTP servlet request
+ * @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 + "[]";
+ ServletFileUpload upload = new ServletFileUpload();
+ List<String> sstParam = new ArrayList<>();
+
+ try {
+ FileItemIterator iter = upload.getItemIterator(request);
+ while (iter.hasNext()) {
+ FileItemStream item = iter.next();
+ if (!item.isFormField()) {
+ continue;
+ }
+
+ if (!item.getFieldName().equals(fieldName)) {
Review Comment:
nit: if conditions can be combined to one.
```suggestion
if (!fieldName.equals(item.getFieldName())
```
to avoid NPE because we know fieldName is constant.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java:
##########
@@ -216,6 +219,68 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) {
}
}
+ /**
+ * Parses request form data parameters.
+ * @param request the HTTP servlet request
+ * @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 + "[]";
Review Comment:
This can be a constant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1578210565
> 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](https://github.com/mladjan-gadzic/ozone/actions/runs/5174385281).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1190467120
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java:
##########
@@ -189,16 +197,19 @@ private void setupCluster() throws Exception {
.thenReturn(cluster.getOzoneManager());
when(requestMock.getParameter(OZONE_DB_CHECKPOINT_REQUEST_FLUSH))
.thenReturn("true");
+ when(requestMock.getMethod()).thenReturn("POST");
+ when(requestMock.getContentType()).thenReturn("multipart/form-data; " +
+ "boundary=" + MULTIPART_FORM_DATA_BOUNDARY);
- doCallRealMethod().when(omDbCheckpointServletMock).doGet(requestMock,
+ doCallRealMethod().when(omDbCheckpointServletMock).doPost(requestMock,
responseMock);
doCallRealMethod().when(omDbCheckpointServletMock)
.writeDbDataToStream(any(), any(), any(), any(), any());
}
@Test
- public void testDoGet() throws Exception {
+ public void testDoPost() throws Exception {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1190464388
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java:
##########
@@ -214,6 +225,26 @@ public void testDoGet() throws Exception {
om.getOmAdminGroups(),
om.isSpnegoEnabled());
+ String crNl = "\r\n";
+ String sstFileName = "sstFile.sst";
+ byte[] data = ("--" + MULTIPART_FORM_DATA_BOUNDARY + crNl +
+ "Content-Disposition: form-data; name=\"" +
+ OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST + "[]\"" + crNl +
+ crNl +
+ sstFileName + crNl +
Review Comment:
please use 2 sst files. (Sometimes testing an array of size 1 hides flaws in loop code.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1555386982
hey @Xushaohong this is these are doPost() changes I discussed with you. Please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1599685243
And thanks @GeorgeJahad and @hemantk-12 for reviewing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1191609702
##########
hadoop-ozone/ozone-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;
Review Comment:
Oh, that is totally a different story. Sure thing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1199435344
##########
hadoop-ozone/ozone-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;
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1594539389
Hi @smengcl. I fixed missing import, merged upstream/master and resolved conflicts. After all of that remote [CI](https://github.com/mladjan-gadzic/ozone/actions/runs/5288561285) passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1586950782
> 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](https://github.com/mladjan-gadzic/ozone/actions/runs/5240441285) succeeded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1190455452
##########
hadoop-ozone/ozone-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;
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1190464875
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/ratis_snapshot/TestOmRatisSnapshotProvider.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.ozone.om.ratis_snapshot;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hadoop.hdds.conf.MutableConfigurationSource;
+import org.apache.hadoop.hdds.utils.LegacyHadoopConfigurationSource;
+import org.apache.hadoop.hdfs.web.URLConnectionFactory;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.om.helpers.OMNodeDetails;
+import org.apache.hadoop.security.authentication.client.AuthenticationException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static java.net.HttpURLConnection.HTTP_OK;
+import static org.apache.hadoop.ozone.OzoneConsts.MULTIPART_FORM_DATA_BOUNDARY;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for {@link OmRatisSnapshotProvider}.
+ */
+public class TestOmRatisSnapshotProvider {
+
+ private OmRatisSnapshotProvider omRatisSnapshotProvider;
+ private OMNodeDetails leader;
+ private String leaderNodeId;
+ private static final String CR_NL = "\r\n";
+ public static final String CONTENT_DISPOSITION =
+ "Content-Disposition: form-data; name=\""
+ + OzoneConsts.OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST + "[]\""
+ + CR_NL + CR_NL;
+ private StringBuilder sb;
+ private File targetFile;
+
+ @BeforeEach
+ public void setup(@TempDir File snapshotDir,
+ @TempDir File downloadDir) throws IOException {
+ targetFile = new File(downloadDir, "newfile");
+
+ MutableConfigurationSource conf =
+ mock(LegacyHadoopConfigurationSource.class);
+ when(conf.get(eq("ozone.http.policy"), any())).thenReturn("HTTP_ONLY");
+ when(conf.get("ozone.om.http.auth.type", "simple"))
+ .thenReturn("no-kerberos");
+
+ Map<String, OMNodeDetails> peerNodesMap = new HashMap<>();
+ leaderNodeId = "1";
+ leader = mock(OMNodeDetails.class);
+ peerNodesMap.put(leaderNodeId, leader);
+
+ omRatisSnapshotProvider =
+ new OmRatisSnapshotProvider(conf, snapshotDir, peerNodesMap);
+
+ sb = new StringBuilder();
+ sb.append("--" + MULTIPART_FORM_DATA_BOUNDARY + CR_NL);
+ sb.append(CONTENT_DISPOSITION);
+ }
+
+ @Test
+ public void testDownloadSnapshot() throws IOException,
+ AuthenticationException {
+ URL omCheckpointUrl = mock(URL.class);
+ when(leader.getOMDBCheckpointEndpointUrl(anyBoolean(), anyBoolean()))
+ .thenReturn(omCheckpointUrl);
+
+ URLConnectionFactory connectionFactory =
+ mock(URLConnectionFactory.class);
+ omRatisSnapshotProvider.setConnectionFactory(connectionFactory);
+
+ HttpURLConnection connection = mock(HttpURLConnection.class);
+ when(connectionFactory.openConnection(any(URL.class), anyBoolean()))
+ .thenReturn(connection);
+
+ ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+ when(connection.getOutputStream()).thenReturn(outputStream);
+ when(connection.getResponseCode()).thenReturn(HTTP_OK);
+ InputStream inputStream =
+ new ByteArrayInputStream(outputStream.toByteArray());
+ when(connection.getInputStream()).thenReturn(inputStream);
+
+ omRatisSnapshotProvider.downloadSnapshot(leaderNodeId, targetFile);
+
+ sb.append("--" + MULTIPART_FORM_DATA_BOUNDARY + "--" + CR_NL);
+ Assertions.assertEquals(sb.toString(),
+ new String(outputStream.toByteArray(), StandardCharsets.UTF_8));
+ }
+
+ @Test
+ public void testWriteFormDataWithSstFile() throws IOException {
+ HttpURLConnection connection = mock(HttpURLConnection.class);
+ List<String> sstFiles = new ArrayList<>();
+ String fileName = "file1.sst";
+ sstFiles.add(fileName);
+ ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+ when(connection.getOutputStream()).thenReturn(outputStream);
+
+
+ OmRatisSnapshotProvider.writeFormData(connection, sstFiles);
+
+ sb.append(fileName).append(CR_NL);
Review Comment:
please use 2 files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1203629405
##########
hadoop-ozone/ozone-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;
Review Comment:
Thanks @hemantk-12 for the review! I added constructor which could be used for testing but also alligns with DI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl merged pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #4695:
URL: https://github.com/apache/ozone/pull/4695
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Xushaohong commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1578312961
> 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](https://github.com/mladjan-gadzic/ozone/tree/HDDS-7922).
You need to update this branch mladjan-gadzic:HDDS-8489
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1191547945
##########
hadoop-ozone/ozone-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;
Review Comment:
I just meant something like this: https://github.com/GeorgeJahad/ozone/compare/da4e99768a..urlConnectionFactory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1191321167
##########
hadoop-ozone/ozone-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;
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on a diff in pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on code in PR #4695:
URL: https://github.com/apache/ozone/pull/4695#discussion_r1226167114
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java:
##########
@@ -216,6 +219,68 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) {
}
}
+ /**
+ * Parses request form data parameters.
+ * @param request the HTTP servlet request
+ * @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 + "[]";
+ ServletFileUpload upload = new ServletFileUpload();
+ List<String> sstParam = new ArrayList<>();
+
+ try {
+ FileItemIterator iter = upload.getItemIterator(request);
+ while (iter.hasNext()) {
+ FileItemStream item = iter.next();
+ if (!item.isFormField()) {
+ continue;
+ }
+
+ if (!item.getFieldName().equals(fieldName)) {
Review Comment:
Conditions are combined and comparation is reversed.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java:
##########
@@ -216,6 +219,68 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) {
}
}
+ /**
+ * Parses request form data parameters.
+ * @param request the HTTP servlet request
+ * @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 + "[]";
Review Comment:
`FIELD_NAME` constant is created.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1583312867
Hi @mladjan-gadzic , would you fix the build failure:
https://github.com/apache/ozone/actions/runs/5066046001/jobs/9137638263?pr=4695
Thx!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mladjan-gadzic commented on pull request #4695: HDDS-8489. Refactor GET request to POST for OM DB snapshot
Posted by "mladjan-gadzic (via GitHub)" <gi...@apache.org>.
mladjan-gadzic commented on PR #4695:
URL: https://github.com/apache/ozone/pull/4695#issuecomment-1578429029
> > 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](https://github.com/mladjan-gadzic/ozone/tree/HDDS-7922). You need to update this branch mladjan-gadzic:[HDDS-8489](https://issues.apache.org/jira/browse/HDDS-8489)
Mistakenly I pasted wrong url. Please, have a look at [https://github.com/mladjan-gadzic/ozone/actions/runs/5066045581](https://github.com/mladjan-gadzic/ozone/actions/runs/5066045581).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org