errose28 commented on code in PR #8523: URL: https://github.com/apache/ozone/pull/8523#discussion_r2232045488
########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/BasicDatanodeInfoJson.java: ########## @@ -0,0 +1,178 @@ +/* + * 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.hdds.scm.cli.datanode; + +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonInclude; +import java.util.List; +import org.apache.hadoop.hdds.protocol.DatanodeDetails; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; + +/** + * Represents filtered Datanode information for json use. + */ + +public class BasicDatanodeInfoJson { + private final String id; + private final String hostName; + private final String ipAddress; + private final List<DatanodeDetails.Port> ports; + private final long setupTime; + private final int currentVersion; + private final HddsProtos.NodeOperationalState persistedOpState; + private final HddsProtos.NodeOperationalState opState; + private final long persistedOpStateExpiryEpochSec; + private final HddsProtos.NodeState healthState; + private final boolean decommissioned; + private final boolean maintenance; + private final int level; + private final int cost; + private final int numOfLeaves; + private final String networkFullPath; + private final String networkLocation; + private final String networkName; + @JsonInclude(JsonInclude.Include.NON_NULL) + private Long used = null; + @JsonInclude(JsonInclude.Include.NON_NULL) + private Long capacity = null; + @JsonInclude(JsonInclude.Include.NON_NULL) + private Double percentUsed = null; + @JsonIgnore + private DatanodeDetails dn; + + public BasicDatanodeInfoJson(DatanodeDetails dnDetails, HddsProtos.NodeOperationalState opState, + HddsProtos.NodeState healthState) { + this.dn = dnDetails; + this.id = dnDetails.getUuid().toString(); Review Comment: We don't need to copy all the fields out of the `DatanodeDetails` here, we can just save the underlying `DatanodeDetails` object as a field. In the getters, we can forward to the getters of the DatanodeDetails. I believe Jackson should use the getters as the definitive list of fields. For example: ``` public String getHostName() { return dnDetails.getHostName(); } ``` Currently we make an unnecessary copy of every field which is never read if the `--json` flag is not passed. ########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/ListInfoSubcommand.java: ########## @@ -155,7 +149,7 @@ private List<DatanodeWithAttributes> getAllNodes(ScmClient scmClient) long capacity = p.getCapacity(); long used = capacity - p.getRemaining(); double percentUsed = (capacity > 0) ? (used * 100.0) / capacity : 0.0; - return new DatanodeWithAttributes( + return new BasicDatanodeInfoJson( Review Comment: I noticed there's an issue above this on line 142 from #8520 that we can fix here. We need to forward the user provided limit on the listing to the server so we don't fetch all the nodes unnecessarily. Right now everything is fetched and then the limit is only applied after by the client. ########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/BasicDatanodeInfoJson.java: ########## @@ -0,0 +1,178 @@ +/* + * 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.hdds.scm.cli.datanode; + +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonInclude; +import java.util.List; +import org.apache.hadoop.hdds.protocol.DatanodeDetails; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; + +/** + * Represents filtered Datanode information for json use. + */ + +public class BasicDatanodeInfoJson { Review Comment: Looking at how this is used now, we probably want to call it `BasicDatanodeInfo` since it is also used outside of the Json case. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
