You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "DaveTeng0 (via GitHub)" <gi...@apache.org> on 2023/11/01 21:12:02 UTC
[PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
DaveTeng0 opened a new pull request, #5530:
URL: https://github.com/apache/ozone/pull/5530
## What changes were proposed in this pull request?
Currently it is possible that a long lived client can add most or all nodes of a small cluster to its exclude list, and further writes using that client instance will fail. This PR add a timeout for RATIS key to remove datanodes from the exclude list so that they can be retried later.
(For EC key, this mechanism exists and is configured to 10 minutes by default.)
This improvement is especially relevant for S3 gateway, which uses a persistent Ozone client to connect to the cluster.
And there is another part of improvement that allows the write to fall back to datanodes in the exclude list if that is all available. This would be implemented as a retry from the client based on the server's initial error response.
This part of work is separated into another PR: https://github.com/apache/ozone/pull/5514
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9323
## How was this patch tested?
unit 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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1397766847
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,151 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ doReturn(true).when(excludeList)
+ .isExpired(anyLong()); // mock DN in exclude list expires
+ keyOutputStream.getBlockOutputStreamEntryPool().setExcludeList(excludeList);
+ Assert.assertEquals(0,
+ keyOutputStream.getExcludeList().getDatanodes().size());
Review Comment:
yeah, you're right.. I change my test strategy to just add one line of assertion in integration test "TestFailureHandlingByClient" to make sure the expiry duration set as default value. Please feel free to let me know if any question! Thanks @adoroszlai ~
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1385592622
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
+ BlockOutputStreamEntryPool streamEntryPool) {
+ blockOutputStreamEntryPool = streamEntryPool;
+ }
+
+ public void setExcludeList(ExcludeList excludeList) {
+ blockOutputStreamEntryPool.setExcludeList(excludeList);
+ }
+
+ protected int getDataWritten(BlockOutputStreamEntry current,
Review Comment:
I guess that this method will only return data written by current block is actually correct.
Since in order to mock the result in test, what I did is just to extract the original code from line 274 here
(https://github.com/apache/ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java#L274) out to an independent method, without any other changes.
And I also take a look at the logic in its caller method "writeToOutputStream(BlockOutputStreamEntry current,
boolean retry, long len, byte[] b, int writeLen, int off, long currentPos)", it does seem to be only for processing the current block, not overall data.
--
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
Re: [PR] HDDS-9323. Apply expiry of excluded datanodes to writing Ratis keys [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5530:
URL: https://github.com/apache/ozone/pull/5530#issuecomment-1821246885
Thanks @DaveTeng0 for the patch, @sumitagrawl for the review.
--
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
Re: [PR] HDDS-9323. Apply expiry of excluded datanodes to writing Ratis keys [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5530:
URL: https://github.com/apache/ozone/pull/5530#issuecomment-1819690892
@DaveTeng0 please merge `master` into your branch, it's 106 commits behind, and there is a conflict
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1387430161
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/OzoneClientConfig.java:
##########
@@ -350,6 +350,10 @@ public long getExcludeNodesExpiryTime() {
return excludeNodesExpiryTime;
}
+ public void setExcludeNodesExpiryTime(long expiryTime) {
Review Comment:
unable to find its caller in changed code, please check if this is required
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,154 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ when(excludeList.getExpiryTime()).thenReturn(300 * 1000L);
+ doReturn(true).when(excludeList)
+ .isExpired(anyLong()); // mock DN in exclude list expires
+ keyOutputStream.getBlockOutputStreamEntryPool().setExcludeList(excludeList);
+ Assert.assertEquals(0,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+ }
+
+ private KeyOutputStream createRATISKeyOutputStream() throws Exception {
+ OpenKeySession openKeySession = mock(OpenKeySession.class);
+ OmKeyInfo omKeyInfo = new OmKeyInfo.Builder()
+ .setVolumeName("testvolume")
+ .setBucketName("testbucket")
+ .setKeyName("testKey")
+ .build();
+ when(openKeySession.getKeyInfo()).thenReturn(omKeyInfo);
+
+ XceiverClientFactory xceiverClientManager
+ = mock(XceiverClientFactory.class);
+
+ OzoneManagerClientProtocol ozoneManagerClientProtocol
+ = mock(OzoneManagerClientProtocol.class);
+
+ OzoneClientConfig clientConfig = spy(new OzoneClientConfig());
Review Comment:
we do not need spy ozone config, just need set config value with new configuration
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,154 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ when(excludeList.getExpiryTime()).thenReturn(300 * 1000L);
Review Comment:
do we really need this now? as now using config, we are setting same value. If not required, we can revert related changes.
--
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
Re: [PR] HDDS-9323. Apply expiry of excluded datanodes to writing Ratis keys [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1399608864
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java:
##########
@@ -117,7 +119,8 @@ public BlockOutputStreamEntryPool(
}
ExcludeList createExcludeList() {
- return new ExcludeList();
+ return new ExcludeList(getConfig().getExcludeNodesExpiryTime(),
+ Clock.system(ZoneOffset.UTC));
Review Comment:
yeah updated.
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1388700185
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,154 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ when(excludeList.getExpiryTime()).thenReturn(300 * 1000L);
+ doReturn(true).when(excludeList)
+ .isExpired(anyLong()); // mock DN in exclude list expires
+ keyOutputStream.getBlockOutputStreamEntryPool().setExcludeList(excludeList);
+ Assert.assertEquals(0,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+ }
+
+ private KeyOutputStream createRATISKeyOutputStream() throws Exception {
+ OpenKeySession openKeySession = mock(OpenKeySession.class);
+ OmKeyInfo omKeyInfo = new OmKeyInfo.Builder()
+ .setVolumeName("testvolume")
+ .setBucketName("testbucket")
+ .setKeyName("testKey")
+ .build();
+ when(openKeySession.getKeyInfo()).thenReturn(omKeyInfo);
+
+ XceiverClientFactory xceiverClientManager
+ = mock(XceiverClientFactory.class);
+
+ OzoneManagerClientProtocol ozoneManagerClientProtocol
+ = mock(OzoneManagerClientProtocol.class);
+
+ OzoneClientConfig clientConfig = spy(new OzoneClientConfig());
Review Comment:
sure! removed spy.
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1388701102
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,154 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ when(excludeList.getExpiryTime()).thenReturn(300 * 1000L);
Review Comment:
yup! you're right, that is redundant! removed. Thanks!
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on PR #5530:
URL: https://github.com/apache/ozone/pull/5530#issuecomment-1806090032
> @DaveTeng0 please check integration test failures:
>
> ```
> org.apache.hadoop.ozone.client.rpc.TestFailureHandlingByClient
> org.apache.hadoop.ozone.client.rpc.TestFailureHandlingByClientFlushDelay
> ```
>
> https://github.com/DaveTeng0/ozone/actions/runs/6818659142/job/18545006173
yup thanks attila! These two tests failure are on my radar!
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1394372324
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,151 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ doReturn(true).when(excludeList)
+ .isExpired(anyLong()); // mock DN in exclude list expires
+ keyOutputStream.getBlockOutputStreamEntryPool().setExcludeList(excludeList);
+ Assert.assertEquals(0,
+ keyOutputStream.getExcludeList().getDatanodes().size());
Review Comment:
The test passes a mocked `ExcludeList` to the `BlockOutputStreamEntryPool`, then makes an assertion on the `ExcludeList` itself. Thus it is testing `ExcludeList`, not `KeyOutputStream`. I don't think all the refactoring (in prod code) and complex test setup (in this class) is necessary to do that.
`ExcludeList` is already tested in `TestExcludeList`. All we need in this PR (as test) is to ensure that the instance created by `BlockOutputStreamEntryPool` has the expiry duration set.
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1384546145
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
+ BlockOutputStreamEntryPool streamEntryPool) {
+ blockOutputStreamEntryPool = streamEntryPool;
+ }
+
+ public void setExcludeList(ExcludeList excludeList) {
Review Comment:
no need expose setExcludeList and excludeList can be final. BlockOutpputStreamEntryPool can return spy of excludeList itself.
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -546,6 +546,7 @@ protected void handleFlush(boolean close) throws IOException {
try {
handleFlushInternal(close);
} catch (ExecutionException e) {
+ failedServers.addAll(getPipeline().getNodes());
Review Comment:
it seems already exclusionList expiry handling is present, just added another scenario here, right?
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
+ BlockOutputStreamEntryPool streamEntryPool) {
+ blockOutputStreamEntryPool = streamEntryPool;
+ }
+
+ public void setExcludeList(ExcludeList excludeList) {
+ blockOutputStreamEntryPool.setExcludeList(excludeList);
+ }
+
+ protected int getDataWritten(BlockOutputStreamEntry current,
Review Comment:
This method will give data written to only current block for which write in progress, but will not give for overall data written.
Need use writeOffset OR can use org.apache.hadoop.ozone.client.io.KeyOutputStream#getLocationInfoList() to return all location and sum-up to get final written length.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
Review Comment:
we can use @VisibleForTesting annotation as used in testcase
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1385792724
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
Review Comment:
sure!
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
+ BlockOutputStreamEntryPool streamEntryPool) {
+ blockOutputStreamEntryPool = streamEntryPool;
+ }
+
+ public void setExcludeList(ExcludeList excludeList) {
Review Comment:
sure!
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1388699922
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/OzoneClientConfig.java:
##########
@@ -350,6 +350,10 @@ public long getExcludeNodesExpiryTime() {
return excludeNodesExpiryTime;
}
+ public void setExcludeNodesExpiryTime(long expiryTime) {
Review Comment:
oh you're right, this is not required. Thanks!
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1385793886
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
+ BlockOutputStreamEntryPool streamEntryPool) {
+ blockOutputStreamEntryPool = streamEntryPool;
+ }
+
+ public void setExcludeList(ExcludeList excludeList) {
+ blockOutputStreamEntryPool.setExcludeList(excludeList);
+ }
+
+ protected int getDataWritten(BlockOutputStreamEntry current,
Review Comment:
I think it makes sense if I rename it as 'getCurrentBlockDataWritten', instead of just 'getDataWritten'
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1397982759
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,151 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ doReturn(true).when(excludeList)
+ .isExpired(anyLong()); // mock DN in exclude list expires
+ keyOutputStream.getBlockOutputStreamEntryPool().setExcludeList(excludeList);
+ Assert.assertEquals(0,
+ keyOutputStream.getExcludeList().getDatanodes().size());
Review Comment:
yeah indeed, thanks @adoroszlai for the suggestion!
--
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
Re: [PR] HDDS-9323. Apply expiry of excluded datanodes to writing Ratis keys [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on PR #5530:
URL: https://github.com/apache/ozone/pull/5530#issuecomment-1819987094
> @DaveTeng0 please merge `master` into your branch, it's 106 commits behind, and there is a conflict
oh sure! working on it!
--
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
Re: [PR] HDDS-9323. Apply expiry of excluded datanodes to writing Ratis keys [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1399199082
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java:
##########
@@ -117,7 +119,8 @@ public BlockOutputStreamEntryPool(
}
ExcludeList createExcludeList() {
- return new ExcludeList();
+ return new ExcludeList(getConfig().getExcludeNodesExpiryTime(),
+ Clock.system(ZoneOffset.UTC));
Review Comment:
new use createExcludeList() for constructor BlockOutputStreamEntryPool() at line 140 as used.
https://github.com/apache/ozone/pull/5530/files#diff-db8e4914806b73e8ec37c15ffd146fc70bd1fcba631df9a314c7389af64a895dL140
--
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
Re: [PR] HDDS-9323. Apply expiry of excluded datanodes to writing Ratis keys [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on PR #5530:
URL: https://github.com/apache/ozone/pull/5530#issuecomment-1819581514
> I think we should apply the same in streaming write:
>
> https://github.com/apache/ozone/blob/2085ebeddb75def4a9ba4ce9ecf3c9d6b4d7584d/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockDataStreamOutputEntryPool.java#L86
oh yes you're right! updated.
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1394372324
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,151 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ doReturn(true).when(excludeList)
+ .isExpired(anyLong()); // mock DN in exclude list expires
+ keyOutputStream.getBlockOutputStreamEntryPool().setExcludeList(excludeList);
+ Assert.assertEquals(0,
+ keyOutputStream.getExcludeList().getDatanodes().size());
Review Comment:
The test passes a new `ExcludeList` to the `BlockOutputStreamEntryPool`, then makes an assertion on the `ExcludeList` itself. Thus it is testing `ExcludeList`, not `KeyOutputStream`. I don't think all the refactoring (in prod code) and complex test setup (in this class) is necessary to do that.
`ExcludeList` is already tested in `TestExcludeList`. All we need in this PR (as test) is to ensure that the instance created by `BlockOutputStreamEntryPool` has the expiry duration set.
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1397841694
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyOutputStream.java:
##########
@@ -0,0 +1,151 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.client.io;
+
+
+import org.apache.hadoop.hdds.client.BlockID;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
+import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerClientProtocol;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test KeyOutputStream with RATIS keys.
+ */
+public class TestKeyOutputStream {
+
+ private static String testKeyString = "test";
+
+ @Test
+ public void testRATISKeyOutputStreamExpiryTime() throws Exception {
+ KeyOutputStream keyOutputStream =
+ createRATISKeyOutputStream();
+ byte[] data = testKeyString.getBytes(UTF_8);
+ keyOutputStream.write(data);
+
+ Assert.assertEquals(3,
+ keyOutputStream.getExcludeList().getDatanodes().size());
+
+ ExcludeList excludeList = spy(keyOutputStream.getExcludeList());
+ doReturn(true).when(excludeList)
+ .isExpired(anyLong()); // mock DN in exclude list expires
+ keyOutputStream.getBlockOutputStreamEntryPool().setExcludeList(excludeList);
+ Assert.assertEquals(0,
+ keyOutputStream.getExcludeList().getDatanodes().size());
Review Comment:
Thanks a lot @DaveTeng0 for updating the patch, I think it's much improved.
--
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
Re: [PR] HDDS-9323. Apply expiry of excluded datanodes to writing Ratis keys [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5530:
URL: https://github.com/apache/ozone/pull/5530
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on PR #5530:
URL: https://github.com/apache/ozone/pull/5530#issuecomment-1817274146
note: there are unit test for ExcludeList.java already in https://github.com/apache/ozone/blob/master/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/scm/container/common/helpers/TestExcludeList.java#L32
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1385581790
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -546,6 +546,7 @@ protected void handleFlush(boolean close) throws IOException {
try {
handleFlushInternal(close);
} catch (ExecutionException e) {
+ failedServers.addAll(getPipeline().getNodes());
Review Comment:
need this line, otherwise exclusionList will always be empty.
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5530:
URL: https://github.com/apache/ozone/pull/5530#issuecomment-1806078376
@DaveTeng0 please check integration test failures:
```
org.apache.hadoop.ozone.client.rpc.TestFailureHandlingByClient
org.apache.hadoop.ozone.client.rpc.TestFailureHandlingByClientFlushDelay
```
https://github.com/DaveTeng0/ozone/actions/runs/6818659142/job/18545006173
--
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
Re: [PR] HDDS-9323. Better datanode exclude list handling for long-lived clients [ozone]
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5530:
URL: https://github.com/apache/ozone/pull/5530#discussion_r1385793886
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -709,4 +709,23 @@ private void checkNotClosed() throws IOException {
+ blockOutputStreamEntryPool.getKeyName());
}
}
+
+ protected BlockOutputStreamEntryPool getBlockOutputStreamEntryPool() {
+ return blockOutputStreamEntryPool;
+ }
+
+ protected void setBlockOutputStreamEntryPool(
+ BlockOutputStreamEntryPool streamEntryPool) {
+ blockOutputStreamEntryPool = streamEntryPool;
+ }
+
+ public void setExcludeList(ExcludeList excludeList) {
+ blockOutputStreamEntryPool.setExcludeList(excludeList);
+ }
+
+ protected int getDataWritten(BlockOutputStreamEntry current,
Review Comment:
I think it makes sense if I rename it as 'getCurrentBlockOutputStreamDataWritten', instead of just 'getDataWritten'
--
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