ozone icon indicating copy to clipboard operation
ozone copied to clipboard

HDDS-10804. Reduce unnecessary Port when AllocateBlock and GetContainerWithPipeline.

Open xichen01 opened this issue 9 months ago • 9 comments

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

  private static final List<PortName> IO_PORTS =
      ImmutableList.of(PortName.PORTNAME_STANDALONE, PortName.PORTNAME_RATIS);

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

xichen01 avatar May 08 '24 10:05 xichen01

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 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 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)?

xichen01 avatar May 09 '24 09:05 xichen01

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.

adoroszlai avatar May 09 '24 10:05 adoroszlai

@adoroszlai I have AllocateBlock and GetContainerWithPipeline return only IO-related ports (STANDALONE, RATIS, RATIS_DATASTREAM).

xichen01 avatar May 12 '24 12:05 xichen01

Thanks @xichen01 for updating the patch. I'd like someone else to also review it.

adoroszlai avatar May 12 '24 18:05 adoroszlai

@adoroszlai PTAL, Thanks.

xichen01 avatar Jun 07 '24 06:06 xichen01

@errose28 please review this for compatibility

adoroszlai avatar Jun 07 '24 07:06 adoroszlai

@adoroszlai @errose28 PTAL, Thanks.

xichen01 avatar Jul 21 '24 07:07 xichen01

@adoroszlai @errose28, Do you have any suggestions on this PR?

xichen01 avatar Oct 02 '24 18:10 xichen01