You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "xichen01 (via GitHub)" <gi...@apache.org> on 2024/05/08 10:23:32 UTC
[PR] HDDS-10804. Reduce unnecessary Port when Create Key. [ozone]
xichen01 opened a new pull request, #6655:
URL: https://github.com/apache/ozone/pull/6655
## What changes were proposed in this pull request?
- The current `DatanodeDetails.Port` of Ozone has a lot of ports, which will cause greater serialization and deserialization pressure (Port#Name is of String type) and may cause high memory consumption.
- When reading/writing an object, most ports are not required,
- This PR will allow the client to declare the required ports when **writing** a request, so that SCM does not need to return all ports to the client.
RpcClient.java
```java
private static final List<PortName> IO_PORTS =
ImmutableList.of(PortName.PORTNAME_STANDALONE, PortName.PORTNAME_RATIS);
```
Can enable this feature by this configuration (default value is `false`)
```xml
ozone.client.limit.port.enable
```
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10804
## How was this patch tested?
Unit test. Existing 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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2106238286
@adoroszlai I have `AllocateBlock` and `GetContainerWithPipeline` return only IO-related ports `(STANDALONE, RATIS, RATIS_DATASTREAM)`.
--
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-10804. Reduce unnecessary Port when Create Key. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2102355148
> Do you think that these ports (`STANDALONE`, `RATIS`, `RATIS_DATASTREA`) is enough for Read request (`getContainerWithPipeline`)?
I think so. `STANDALONE` is used for read and EC write, the other two for Ratis write.
--
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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #6655:
URL: https://github.com/apache/ozone/pull/6655#discussion_r1844913451
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -448,11 +449,15 @@ public HddsProtos.DatanodeDetailsProto getProtoBufMessage() {
}
public HddsProtos.DatanodeDetailsProto toProto(int clientVersion) {
- return toProtoBuilder(clientVersion).build();
+ return toProtoBuilder(clientVersion, Collections.emptySet()).build();
+ }
+
+ public HddsProtos.DatanodeDetailsProto toProto(int clientVersion, Set<Port.Name> requiredPorts) {
+ return toProtoBuilder(clientVersion, requiredPorts).build();
}
public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
- int clientVersion) {
+ int clientVersion, Set<Port.Name> requiredPorts) {
Review Comment:
Nit: Let's add javadocs here and specify that empty set means that all ports will be included.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -492,14 +497,19 @@ public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
ClientVersion.fromProtoValue(clientVersion)
.compareTo(VERSION_HANDLES_UNKNOWN_DN_PORTS) >= 0;
for (Port port : ports) {
- if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
- builder.addPorts(port.toProto());
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Skip adding {} port {} to proto message for client v{}",
- port.getName(), port.getValue(), clientVersion);
+ if (requiredPorts.isEmpty() || requiredPorts.contains(port.name)) {
+ if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
+ builder.addPorts(port.toProto());
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Skip adding {} port {} to proto message for client v{}",
+ port.getName(), port.getValue(), clientVersion);
+ }
}
}
Review Comment:
Nit: Let's add another debug log in an `else` block for the case where the non required ports are skipped.
--
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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6655:
URL: https://github.com/apache/ozone/pull/6655#discussion_r1844935071
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -492,14 +497,19 @@ public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
ClientVersion.fromProtoValue(clientVersion)
.compareTo(VERSION_HANDLES_UNKNOWN_DN_PORTS) >= 0;
for (Port port : ports) {
- if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
- builder.addPorts(port.toProto());
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Skip adding {} port {} to proto message for client v{}",
- port.getName(), port.getValue(), clientVersion);
+ if (requiredPorts.isEmpty() || requiredPorts.contains(port.name)) {
+ if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
+ builder.addPorts(port.toProto());
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Skip adding {} port {} to proto message for client v{}",
+ port.getName(), port.getValue(), clientVersion);
+ }
}
}
Review Comment:
My [suggestion](https://github.com/apache/ozone/pull/6655#pullrequestreview-2045531935) included that, plus two minor optimizations:
- check whether all ports are needed (`requiredPorts.isEmpty()`) once, before the loop
- exit from loop if found all requested ports
It would be nice to adopt these.
```java
final int requestedPortCount = requiredPorts.size();
final boolean maySkip = requestedPortCount > 0;
for (Port port : ports) {
if (maySkip && !requiredPorts.contains(port.getName())) {
if (LOG.isDebugEnabled()) {
LOG.debug("Skip adding {} port {} to proto message",
port.getName(), port.getValue());
}
} else if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
builder.addPorts(port.toProto());
} else {
if (LOG.isDebugEnabled()) {
LOG.debug("Skip adding {} port {} to proto message for client v{}",
port.getName(), port.getValue(), clientVersion);
}
}
if (maySkip && builder.getPortsCount() == requestedPortCount) {
break;
}
}
```
https://github.com/adoroszlai/ozone/blob/dce755c4f3a3e409dfc4b43bc16a48cf2a068842/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java#L458-L477
--
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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2154199460
@adoroszlai PTAL, 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-10804. Reduce unnecessary Port when Create Key. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2102329910
> Thanks @xichen01 for working on this.
>
> Can SCM simply always send a limited set of ports (`STANDALONE, RATIS, RATIS_DATASTREAM`) in response to `AllocateBlock` requests? I don't think client needs any other ports.
>
> It would let us avoid:
>
> 1. new config
> 2. new proto for port names
> 3. requiring client to send port names
> 4. changing OM requests
>
> patch: [master...adoroszlai:ozone:dce755c4f3a3e409dfc4b43bc16a48cf2a068842](https://github.com/apache/ozone/compare/master...adoroszlai:ozone:dce755c4f3a3e409dfc4b43bc16a48cf2a068842) CI: https://github.com/adoroszlai/ozone/actions/runs/9004107560
>
> > ## How was this patch tested?
> > Unit test. Existing test
>
> To let existing tests cover the changes, we need to set `ozone.client.limit.port.enable=true`. Since the default setting is `false`, they only cover old behavior. Please see failures found by [run with `true`](https://github.com/adoroszlai/ozone/actions/runs/9000547866) in my fork. Some of the failures are due to `RATIS_DATASTREAM` port missing from the limited set.
Good idea, I will update this. Do you think that these ports (`STANDALONE`, `RATIS`, `RATIS_DATASTREA`) is enough for Read request (`getContainerWithPipeline`)?
--
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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2419942200
@xichen01 I hope we can find someone to review 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-10804. Include only limited set of ports in Pipeline proto [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2482142568
Thanks @xichen01 for the patch, @ivandika3 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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2106343259
Thanks @xichen01 for updating the patch. I'd like someone else to also review 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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2241516332
@adoroszlai @errose28 PTAL, 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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #6655:
URL: https://github.com/apache/ozone/pull/6655#discussion_r1844965891
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -448,11 +449,15 @@ public HddsProtos.DatanodeDetailsProto getProtoBufMessage() {
}
public HddsProtos.DatanodeDetailsProto toProto(int clientVersion) {
- return toProtoBuilder(clientVersion).build();
+ return toProtoBuilder(clientVersion, Collections.emptySet()).build();
+ }
+
+ public HddsProtos.DatanodeDetailsProto toProto(int clientVersion, Set<Port.Name> requiredPorts) {
+ return toProtoBuilder(clientVersion, requiredPorts).build();
}
public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
- int clientVersion) {
+ int clientVersion, Set<Port.Name> requiredPorts) {
Review Comment:
`requiredPorts` does tend to be misleading I have changed it to `filterPorts`
--
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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2154250764
@errose28 please review this for compatibility
--
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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #6655:
URL: https://github.com/apache/ozone/pull/6655#issuecomment-2480415471
> Do you think that these ports (STANDALONE, RATIS, RATIS_DATASTREAM) is enough for Read request (getContainerWithPipeline)?
Currently since RATIS_DATASTREAM is only used for writes, we can technically exclude it. However, there might be plan in the future to support streaming read (https://issues.apache.org/jira/browse/HDDS-9904).
--
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-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #6655:
URL: https://github.com/apache/ozone/pull/6655#discussion_r1844965915
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -492,14 +497,19 @@ public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
ClientVersion.fromProtoValue(clientVersion)
.compareTo(VERSION_HANDLES_UNKNOWN_DN_PORTS) >= 0;
for (Port port : ports) {
- if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
- builder.addPorts(port.toProto());
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Skip adding {} port {} to proto message for client v{}",
- port.getName(), port.getValue(), clientVersion);
+ if (requiredPorts.isEmpty() || requiredPorts.contains(port.name)) {
+ if (handlesUnknownPorts || Name.V0_PORTS.contains(port.getName())) {
+ builder.addPorts(port.toProto());
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Skip adding {} port {} to proto message for client v{}",
+ port.getName(), port.getValue(), clientVersion);
+ }
}
}
Review Comment:
OK, 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-10804. Include only limited set of ports in Pipeline proto [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #6655:
URL: https://github.com/apache/ozone/pull/6655
--
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