You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by GitBox <gi...@apache.org> on 2022/12/27 11:20:49 UTC
[GitHub] [incubator-uniffle] advancedxy opened a new pull request, #449: WIP: shuffle server report storage capability
advancedxy opened a new pull request, #449:
URL: https://github.com/apache/incubator-uniffle/pull/449
### What changes were proposed in this pull request?
ShuffleServer reports storage info about itself.
### Why are the changes needed?
To do better shuffle assignments and get more insight of shuffle server
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Will add UTs.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064486454
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
Could we have an another name?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064142486
##########
storage/src/main/java/org/apache/uniffle/storage/common/DefaultStorageTypeProvider.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.uniffle.storage.common;
+
+import org.apache.uniffle.common.storage.SingleStorageType;
+
+public class DefaultStorageTypeProvider implements StorageTypeProvider {
+ @Override
+ public SingleStorageType getStorageTypeFor(String baseDir) {
+ return SingleStorageType.HDD;
Review Comment:
An update:
Just tried the command on two azure vm instances, one has azuere's standard ssd, another has HDD.
The `/sys/block/sdx/queue/rotational` both gives 1 for all disks.
So, I believe this command may work well for physical host, but doesn't suit for other deployments.
However it might be still be a good default way to provide disk type info.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065313225
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
> > If they have diferent package names, there isn't conflict names.
>
> Unlike scala, java doesn't support class name renaming in the import path, and you cannot import the same class name in one file even if they are from different packages. It's extremely strange to use a fully qualified class name in java file and that always indicates some code smell. And FYI, I have renamed all the `StorageTypePOJO` to `StorageMedia` due to @zuston's suggestion, which I think it's a better name.
>
> P.S. you cannot do the following in one java file
>
> ```
> import org.apache.uniffle.common.storage.StorageType
> import org.apache.uniffle.storage.util.StorageType
> ```
but you don't need import any of them.
You can use org.apache.uniffle.common.storage.StorageType and org.apache.uniffle.storage.util.StorageType directly.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi merged PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064313252
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
> Do we need 1M 5M 1H for failure number？
Replied in design doc
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064419642
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
POJO is the concept of `Java bean`. I don't understand why we introduce this concept.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064510941
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -94,19 +101,26 @@ public class LocalStorageManager extends SingleStorageManager {
// We must make sure the order of `storageBasePaths` and `localStorages` is same, or some unit test may be fail
CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.size());
AtomicInteger successCount = new AtomicInteger();
+ ServiceLoader<StorageTypeProvider> loader = ServiceLoader.load(StorageTypeProvider.class);
Review Comment:
> Could you help provide some examples about needing multiple providers?
Just like the `DefaultStorageTypeProvider` and `StorageTypeFromEnvProvider`. Some base dir might be indeed is a SSD disk but the default could only return HDD>
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065527590
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageMediaFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.StorageMedia;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageMediaProvider;
+
+public class StorageMediaFromEnvProvider implements StorageMediaProvider {
+ private static Logger logger = LoggerFactory.getLogger(StorageMediaFromEnvProvider.class);
+ private Map<String, StorageMedia> localStorageTypes = Maps.newHashMap();
+
+ /**
+ * Returns current storage type for baseDir.
+ *
+ * @param baseDir the storage dir to check for
+ * @return LocalStorageType if valid or queryable. Unknown would be returned if it cannot be determined.
+ */
+ @Override
+ public StorageMedia getStorageMediaFor(String baseDir) {
+ File f = new File(baseDir);
+ while (f != null) {
+ if (localStorageTypes.containsKey(f.getPath())) {
+ return localStorageTypes.get(f.getPath());
+ }
+ f = f.getParentFile();
+ }
+ return StorageMedia.UNKNOWN;
+ }
+
+ @Override
+ public void init(RssConf conf) {
+ String envKey = conf.get(ShuffleServerConf.STORAGE_MEDIA_PROVIDER_ENV_KEY);
Review Comment:
For other systems, do they also use the json for similar situation?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065562681
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageMediaFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.StorageMedia;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageMediaProvider;
+
+public class StorageMediaFromEnvProvider implements StorageMediaProvider {
+ private static Logger logger = LoggerFactory.getLogger(StorageMediaFromEnvProvider.class);
+ private Map<String, StorageMedia> localStorageTypes = Maps.newHashMap();
+
+ /**
+ * Returns current storage type for baseDir.
+ *
+ * @param baseDir the storage dir to check for
+ * @return LocalStorageType if valid or queryable. Unknown would be returned if it cannot be determined.
+ */
+ @Override
+ public StorageMedia getStorageMediaFor(String baseDir) {
+ File f = new File(baseDir);
+ while (f != null) {
+ if (localStorageTypes.containsKey(f.getPath())) {
+ return localStorageTypes.get(f.getPath());
+ }
+ f = f.getParentFile();
+ }
+ return StorageMedia.UNKNOWN;
+ }
+
+ @Override
+ public void init(RssConf conf) {
+ String envKey = conf.get(ShuffleServerConf.STORAGE_MEDIA_PROVIDER_ENV_KEY);
Review Comment:
Yeah, mostly. Json is very convenient and commonly used. Even other format is used, we can simply extend this provider to support that.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] a-li commented on a diff in pull request #449: WIP: shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
a-li commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1058114284
##########
common/src/main/java/org/apache/uniffle/common/storage/LocalStorageInfoUtils.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.uniffle.common.storage;
+
+import com.google.common.collect.Maps;
+import org.apache.uniffle.proto.RssProtos;
+
+import java.util.Map;
+
+public class LocalStorageInfoUtils {
+ public static Map<String, RssProtos.LocalStorageInfo> toProto(
+ Map<String, LocalStorageInfo> info) {
+ Map<String, RssProtos.LocalStorageInfo> result = Maps.newHashMapWithExpectedSize(info.size());
+ info.forEach((k, v) -> result.put(k, v.toProto()));
+ return result;
+ }
+
+ public static Map<String, LocalStorageInfo> fromProto(Map<String, RssProtos.LocalStorageInfo> info) {
+ Map<String, LocalStorageInfo> result = Maps.newHashMapWithExpectedSize(info.size());
+ for (Map.Entry<String, RssProtos.LocalStorageInfo> entry : info.entrySet()) {
+ String key = entry.getKey();
+ RssProtos.LocalStorageInfo val = entry.getValue();
+ LocalStorageInfo localStorageInfo = new LocalStorageInfo(
+ val.getMountPoint(),
+ LocalStorageType.fromProto(val.getStorageType()),
+ val.getCapacity(),
+ val.getUsedBytes(),
+ LocalStorageStatus.fromProto(val.getStatus()));
+ }
Review Comment:
Is it missing a `result.put` call here inside the for loop?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065507373
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageInfo.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.uniffle.common.storage;
+
+import java.util.Objects;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public class StorageInfo {
+ private String mountPoint;
+ private StorageMedia type;
+ private long capacity;
+ private long usedBytes;
+ // -1 indicates these field is not used and shall not be serialized to proto.
+ private long writingSpeed1M;
+ private long writingSpeed5M;
+ private long writingSpeed1H;
+ private long numberOfWritingFailures;
+ private StorageStatus status;
+
+ public StorageInfo(
+ String mountPoint,
+ StorageMedia type,
+ long capacity,
+ long usedBytes,
+ StorageStatus status) {
+ this.mountPoint = mountPoint;
+ this.type = type;
+ this.capacity = capacity;
+ this.usedBytes = usedBytes;
+ this.writingSpeed1M = -1;
+ this.writingSpeed5M = -1;
+ this.writingSpeed1H = -1;
+ this.numberOfWritingFailures = -1;
+ this.status = status;
+ }
+
+ public StorageInfo(
+ String mountPoint,
+ StorageMedia type,
+ long capacity,
+ long usedBytes,
+ long writingSpeed1M,
+ long writingSpeed5M,
+ long writingSpeed1H,
+ long numberOfWritingFailures,
+ StorageStatus status) {
+ this.mountPoint = mountPoint;
+ this.type = type;
+ this.capacity = capacity;
+ this.usedBytes = usedBytes;
+ this.writingSpeed1M = writingSpeed1M;
+ this.writingSpeed5M = writingSpeed5M;
+ this.writingSpeed1H = writingSpeed1H;
+ this.numberOfWritingFailures = numberOfWritingFailures;
+ this.status = status;
+ }
+
+
+ public RssProtos.StorageInfo toProto() {
+ RssProtos.StorageInfo.Builder builder = RssProtos.StorageInfo.newBuilder()
+ .setMountPoint(mountPoint)
+ .setStorageMedia(type.toProto())
+ .setCapacity(capacity)
+ .setUsedBytes(usedBytes)
+ .setStatus(status.toProto());
+ if (writingSpeed1M >= 0) {
+ builder.setWritingSpeed1M(writingSpeed1M);
+ builder.setWritingSpeed5M(writingSpeed5M);
+ builder.setWritingSpeed1H(writingSpeed1H);
+ }
+
+ if (numberOfWritingFailures >= 0) {
+ builder.setNumOfWritingFailures(numberOfWritingFailures);
+ }
+
+ return builder.build();
+ }
+
+ public StorageStatus getStatus() {
+ return status;
+ }
+
+ public StorageMedia getType() {
+ return type;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ StorageInfo that = (StorageInfo) o;
+ return Objects.equals(mountPoint, that.mountPoint) && type == that.type
+ && capacity == that.capacity
+ && usedBytes == that.usedBytes
+ && status == that.status;
+ }
+
+ @Override
+ public int hashCode() {
Review Comment:
Forget that. will address later.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1060505648
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
> Maybe we should collect remote storage, too. The failure times of writing is also meaningful. So we should give a more comprehensive design.
Yeah, remote storage info is also useful. Let me write a quick design doc and let's discuss there.
> why not directly using repeated LocalStorageInfo localStorages
For local storages, there might be multiple path pointing to the same mount point. I'd like to do some deduplicate here.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064139280
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
+ Unknown(0),
Review Comment:
Nice catch.
Will address in later.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064479454
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, StorageInfo> localStorages = 21; // mount point to storage info mapping.
Review Comment:
Got 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#issuecomment-1366575760
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/449?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#449](https://codecov.io/gh/apache/incubator-uniffle/pull/449?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6922976) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/6ddf8a7f0ae7069b869e5734c40226ac2c81494d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6ddf8a7) will **decrease** coverage by `0.50%`.
> The diff coverage is `69.82%`.
```diff
@@ Coverage Diff @@
## master #449 +/- ##
============================================
- Coverage 58.71% 58.20% -0.51%
+ Complexity 1651 1463 -188
============================================
Files 199 184 -15
Lines 11214 9539 -1675
Branches 998 851 -147
============================================
- Hits 6584 5552 -1032
+ Misses 4237 3621 -616
+ Partials 393 366 -27
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/uniffle/coordinator/CoordinatorGrpcService.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ29vcmRpbmF0b3JHcnBjU2VydmljZS5qYXZh) | `2.08% <0.00%> (-0.02%)` | :arrow_down: |
| [...che/uniffle/server/storage/HdfsStorageManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9zdG9yYWdlL0hkZnNTdG9yYWdlTWFuYWdlci5qYXZh) | `93.75% <0.00%> (-1.49%)` | :arrow_down: |
| [...he/uniffle/server/storage/MultiStorageManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9zdG9yYWdlL011bHRpU3RvcmFnZU1hbmFnZXIuamF2YQ==) | `62.96% <0.00%> (-1.19%)` | :arrow_down: |
| [...fle/storage/common/DefaultStorageTypeProvider.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2NvbW1vbi9EZWZhdWx0U3RvcmFnZVR5cGVQcm92aWRlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...he/uniffle/storage/common/StorageTypeProvider.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2NvbW1vbi9TdG9yYWdlVHlwZVByb3ZpZGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...che/uniffle/common/storage/LocalStorageStatus.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9zdG9yYWdlL0xvY2FsU3RvcmFnZVN0YXR1cy5qYXZh) | `45.00% <45.00%> (ø)` | |
| [...pache/uniffle/common/storage/LocalStorageType.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9zdG9yYWdlL0xvY2FsU3RvcmFnZVR5cGUuamF2YQ==) | `50.00% <50.00%> (ø)` | |
| [...pache/uniffle/common/storage/LocalStorageInfo.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9zdG9yYWdlL0xvY2FsU3RvcmFnZUluZm8uamF2YQ==) | `51.72% <51.72%> (ø)` | |
| [...he/uniffle/server/storage/LocalStorageManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9zdG9yYWdlL0xvY2FsU3RvcmFnZU1hbmFnZXIuamF2YQ==) | `85.87% <74.19%> (-2.49%)` | :arrow_down: |
| [...fle/server/storage/StorageTypeFromEnvProvider.java](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9zdG9yYWdlL1N0b3JhZ2VUeXBlRnJvbUVudlByb3ZpZGVyLmphdmE=) | `89.65% <89.65%> (ø)` | |
| ... and [32 more](https://codecov.io/gh/apache/incubator-uniffle/pull/449/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065635636
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
Review Comment:
Could we use `StorageInfo` to replace `RssProtos.StorageInfo`? Is this style consistent with other places?
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
this.id = id;
Review Comment:
Is this feature a compatible feature? Can coordinator recognize the old server's request?
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -336,6 +336,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription("Threshold when flushing shuffle data to persistent storage, recommend value would be 256K, "
+ "512K, or even 1M");
+ public static final ConfigOption<String> STORAGE_MEDIA_PROVIDER_ENV_KEY = ConfigOptions
+ .key("rss.server.localStorageProvider.from.env.key")
Review Comment:
Could we modify the key name?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065681825
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
Review Comment:
No. `ServerNode` also import `org.apache.uniffle.common.storage.StorageInfo`, which is the same name with `RssProtos.StorageInfo`.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065614569
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -94,19 +101,26 @@ public class LocalStorageManager extends SingleStorageManager {
// We must make sure the order of `storageBasePaths` and `localStorages` is same, or some unit test may be fail
CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.size());
AtomicInteger successCount = new AtomicInteger();
+ ServiceLoader<StorageTypeProvider> loader = ServiceLoader.load(StorageTypeProvider.class);
Review Comment:
I can't agree this, we could use single provider to mark one storage media for one basePath, this is enough.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065792495
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
Review Comment:
Good suggestion. 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1058706804
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
Emm, maybe this is helpful..
But remote storage isn't part of ShuffleServer, coordinator cannot use this info to do something meaningful.
Let me reconsider this 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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065268008
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
> > POJO is the concept of Java bean. I don't understand why we introduce this concept.
>
> POJO is a not Java Bean, although it's always compared to Java Bean. The main reason to add suffix here is to avoid name conflicting, and the wrapper class for pb message falls into the POJO category.
Why do we have the name 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065272304
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
StorageType
> > > POJO is the concept of Java bean. I don't understand why we introduce this concept.
> >
> >
> > POJO is a not Java Bean, although it's always compared to Java Bean. The main reason to add suffix here is to avoid name conflicting, and the wrapper class for pb message falls into the POJO category.
>
> Why do we have the name conflict?
StorageType is also defined in `org.apache.uniffle.storage.util`, which conflicts this class name.
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
> `storageMedia` ?
Nice 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065311345
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
> If they have diferent package names, there isn't conflict names.
Unlike scala, java doesn't support class name renaming in the import path, and you cannot import the same class name in one file even if they are from different packages. It's extremely strange to use a fully qualified class name in java file and that always indicates some code smell.
And FYI, I have renamed all the `StorageTypePOJO` to `StorageMedia` due to @zuston's suggestion, which I think it's a better name.
P.S. you cannot do the following in one java file
```
import org.apache.uniffle.common.storage.StorageType
import org.apache.uniffle.storage.util.StorageType
```
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1063265004
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
+ Unknown(0),
Review Comment:
Unknown -> `UNKNOWN?`
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
+ Unknown(0),
+ HDD(1),
+ SSD(2),
+ HDFS(3),
+ OBJECT(4);
Review Comment:
OBJECT -> `OBJECT_STORE` ?
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
+ Unknown(0),
+ HDD(1),
+ SSD(2),
+ HDFS(3),
+ OBJECT(4);
+
+ private final byte val;
+
+ SingleStorageType(int code) {
+ assert (code >= -1 && code < 256);
+ this.val = (byte) code;
+ }
+
+ public RssProtos.StorageInfo.SingleStorageType toProto() {
+ switch (this) {
+ case Unknown:
+ return RssProtos.StorageInfo.SingleStorageType.STORAGE_TYPE_UNKNOWN;
+ case HDD:
+ return RssProtos.StorageInfo.SingleStorageType.HDD;
+ case SSD:
+ return RssProtos.StorageInfo.SingleStorageType.SSD;
+ case HDFS:
+ return RssProtos.StorageInfo.SingleStorageType.HDFS;
+ case OBJECT:
+ return RssProtos.StorageInfo.SingleStorageType.OBJECT;
+ default:
+ return RssProtos.StorageInfo.SingleStorageType.UNRECOGNIZED;
Review Comment:
What's the different with `UNRECOGNIZED` and `STORAGE_TYPE_UNKNOWN` ?
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageStatus.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum StorageStatus {
+ UNKNOWN(0),
+ NORMAL(1),
+ UNHEALTHY(2),
Review Comment:
What will make status unhealthy ? For example Disk I/O is too high?
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, StorageInfo> localStorages = 21; // mount point to storage info mapping.
Review Comment:
Why the proto var number is 21 instead of 8?
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageStatus.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum StorageStatus {
+ UNKNOWN(0),
+ NORMAL(1),
+ UNHEALTHY(2),
+ OVERUSED(3);
+
+ private final byte val;
+
+ StorageStatus(int code) {
+ assert (code >= -1 && code < 256);
+ this.val = (byte) code;
+ }
+
+ public final byte getCode() {
+ return val;
+ }
+
+ public RssProtos.StorageInfo.StorageStatus toProto() {
+ switch (this) {
+ case UNKNOWN:
+ return RssProtos.StorageInfo.StorageStatus.STORAGE_STATUS_UNKNOWN;
+ case NORMAL:
+ return RssProtos.StorageInfo.StorageStatus.NORMAL;
+ case UNHEALTHY:
+ return RssProtos.StorageInfo.StorageStatus.UNHEALTHY;
+ case OVERUSED:
+ return RssProtos.StorageInfo.StorageStatus.OVERUSED;
+ default:
+ return RssProtos.StorageInfo.StorageStatus.UNRECOGNIZED;
Review Comment:
What's the different with `STORAGE_STATUS_UNKNOWN` and `UNRECOGNIZED`
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -94,19 +101,26 @@ public class LocalStorageManager extends SingleStorageManager {
// We must make sure the order of `storageBasePaths` and `localStorages` is same, or some unit test may be fail
CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.size());
AtomicInteger successCount = new AtomicInteger();
+ ServiceLoader<StorageTypeProvider> loader = ServiceLoader.load(StorageTypeProvider.class);
Review Comment:
Why we need multiple storage type providers? I think one is enough.
##########
storage/src/main/java/org/apache/uniffle/storage/common/DefaultStorageTypeProvider.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.uniffle.storage.common;
+
+import org.apache.uniffle.common.storage.SingleStorageType;
+
+public class DefaultStorageTypeProvider implements StorageTypeProvider {
+ @Override
+ public SingleStorageType getStorageTypeFor(String baseDir) {
+ return SingleStorageType.HDD;
Review Comment:
I think this could be as default storage type provider, which could find out the storage type by linux command, https://unix.stackexchange.com/questions/65595/how-to-know-if-a-disk-is-an-ssd-or-an-hdd
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
Review Comment:
SingleStorageType rename to `UnderlyingStorageType` ? The word of `Single` looks general, and the name of `StorageType` has been occupied.
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageTypeFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.SingleStorageType;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageTypeProvider;
+
+public class StorageTypeFromEnvProvider implements StorageTypeProvider {
Review Comment:
I think the storage type mapping in env is too implict. Why not using config file?
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageTypeFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.SingleStorageType;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageTypeProvider;
+
+public class StorageTypeFromEnvProvider implements StorageTypeProvider {
Review Comment:
If this is only used for test, it's ok for me.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064139878
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
Review Comment:
Yeah... Originally `SingleStorageType` is defined as `StorageType`, but I found it's conflict with another `StorageType` class.
Let me consider the `UnderlyingStorageType` naming, it seems a bit cumbersome though.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064414270
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
Not for now.
But for style consistence purpose, we may refactor these wrapper class with POJO suffix.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064139603
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
+ Unknown(0),
+ HDD(1),
+ SSD(2),
+ HDFS(3),
+ OBJECT(4);
+
+ private final byte val;
+
+ SingleStorageType(int code) {
+ assert (code >= -1 && code < 256);
+ this.val = (byte) code;
+ }
+
+ public RssProtos.StorageInfo.SingleStorageType toProto() {
+ switch (this) {
+ case Unknown:
+ return RssProtos.StorageInfo.SingleStorageType.STORAGE_TYPE_UNKNOWN;
+ case HDD:
+ return RssProtos.StorageInfo.SingleStorageType.HDD;
+ case SSD:
+ return RssProtos.StorageInfo.SingleStorageType.SSD;
+ case HDFS:
+ return RssProtos.StorageInfo.SingleStorageType.HDFS;
+ case OBJECT:
+ return RssProtos.StorageInfo.SingleStorageType.OBJECT;
+ default:
+ return RssProtos.StorageInfo.SingleStorageType.UNRECOGNIZED;
Review Comment:
UNRECOGNIZED is generated by default from protoc, which doesn't behave like a normal enum, for example it doesn't support getter for `UNRECOGNIZED`
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064141669
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageTypeFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.SingleStorageType;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageTypeProvider;
+
+public class StorageTypeFromEnvProvider implements StorageTypeProvider {
Review Comment:
This is specially tailed for some container deployment, which providing mapping info through env.
I didn't see too much difference between reading from env or from config file.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065339135
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
> You can use org.apache.uniffle.common.storage.StorageType and org.apache.uniffle.storage.util.StorageType directly.
Yeah. But that seems quite strange and I don't think it's a good coding style.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065694790
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
this.id = id;
Review Comment:
Yes. the `storageInfoMap` should be an empty Map when deserializing from old code.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -336,6 +336,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription("Threshold when flushing shuffle data to persistent storage, recommend value would be 256K, "
+ "512K, or even 1M");
+ public static final ConfigOption<String> STORAGE_MEDIA_PROVIDER_ENV_KEY = ConfigOptions
+ .key("rss.server.localStorageProvider.from.env.key")
Review Comment:
OK.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065510301
##########
common/pom.xml:
##########
@@ -83,6 +83,12 @@
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
+ <exclusions>
Review Comment:
hadoop-common introduces protobuf-2.5, which conflicts with protobuf 3.
We need to exclude 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065708181
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
this.id = id;
Review Comment:
OK.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1058689270
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
What properties do you want to have for RemoteStorageInfo? I thought about that when implementing this PR, but didn't find anything meaning and general enough. If you have some ideas/suggestions, I'd appreciate that.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064509112
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
What do you have in mind, other than `UnderlyingStorageType`? I'm ok with that if there's no better name. But to be honest, this naming is quite cumbersome.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065273849
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
> StorageType
>
> > > > POJO is the concept of Java bean. I don't understand why we introduce this concept.
> > >
> > >
> > > POJO is a not Java Bean, although it's always compared to Java Bean. The main reason to add suffix here is to avoid name conflicting, and the wrapper class for pb message falls into the POJO category.
> >
> >
> > Why do we have the name conflict?
>
> StorageType is also defined in `org.apache.uniffle.storage.util`, which conflicts this class name.
POJO reminds me that this may be a java bean, it's weird for me. If they have diferent package names, there isn't conflict names.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064140083
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageStatus.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum StorageStatus {
+ UNKNOWN(0),
+ NORMAL(1),
+ UNHEALTHY(2),
Review Comment:
Currently, when `LocalStorage` is corrupted, we can extend it to other conditions, but it should be addressed in other PRs.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064140363
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, StorageInfo> localStorages = 21; // mount point to storage info mapping.
Review Comment:
I believe there might be other properties more important for storages properties.
So I reserved some fields numbering.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064399730
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
For other classes, is there similar problems?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065616414
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageMediaFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.StorageMedia;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageMediaProvider;
+
+public class StorageMediaFromEnvProvider implements StorageMediaProvider {
+ private static Logger logger = LoggerFactory.getLogger(StorageMediaFromEnvProvider.class);
+ private Map<String, StorageMedia> localStorageTypes = Maps.newHashMap();
+
+ /**
+ * Returns current storage type for baseDir.
+ *
+ * @param baseDir the storage dir to check for
+ * @return LocalStorageType if valid or queryable. Unknown would be returned if it cannot be determined.
+ */
+ @Override
+ public StorageMedia getStorageMediaFor(String baseDir) {
+ File f = new File(baseDir);
+ while (f != null) {
+ if (localStorageTypes.containsKey(f.getPath())) {
+ return localStorageTypes.get(f.getPath());
+ }
+ f = f.getParentFile();
+ }
+ return StorageMedia.UNKNOWN;
+ }
+
+ @Override
+ public void init(RssConf conf) {
+ String envKey = conf.get(ShuffleServerConf.STORAGE_MEDIA_PROVIDER_ENV_KEY);
Review Comment:
I think Json is easy to configure and see in K8s pod/statefulset yaml.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064313504
##########
storage/src/main/java/org/apache/uniffle/storage/common/DefaultStorageTypeProvider.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.uniffle.storage.common;
+
+import org.apache.uniffle.common.storage.SingleStorageType;
+
+public class DefaultStorageTypeProvider implements StorageTypeProvider {
+ @Override
+ public SingleStorageType getStorageTypeFor(String baseDir) {
+ return SingleStorageType.HDD;
Review Comment:
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1061175925
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
https://docs.google.com/document/d/1JcXJxwmJWCtNMWb8bS9LdcKz2x7kqSyxDjKt3BcPZmk/edit?usp=sharing
@jerqi @zuston I wrote a quick design doc, please commented there if you have more input.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065366762
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
PTAL @jerqi @zuston
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065526293
##########
common/pom.xml:
##########
@@ -83,6 +83,12 @@
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
+ <exclusions>
Review Comment:
OK.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065374266
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageMediaFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.StorageMedia;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageMediaProvider;
+
+public class StorageMediaFromEnvProvider implements StorageMediaProvider {
+ private static Logger logger = LoggerFactory.getLogger(StorageMediaFromEnvProvider.class);
+ private Map<String, StorageMedia> localStorageTypes = Maps.newHashMap();
+
+ /**
+ * Returns current storage type for baseDir.
+ *
+ * @param baseDir the storage dir to check for
+ * @return LocalStorageType if valid or queryable. Unknown would be returned if it cannot be determined.
+ */
+ @Override
+ public StorageMedia getStorageMediaFor(String baseDir) {
+ File f = new File(baseDir);
+ while (f != null) {
+ if (localStorageTypes.containsKey(f.getPath())) {
+ return localStorageTypes.get(f.getPath());
+ }
+ f = f.getParentFile();
+ }
+ return StorageMedia.UNKNOWN;
+ }
+
+ @Override
+ public void init(RssConf conf) {
+ String envKey = conf.get(ShuffleServerConf.STORAGE_MEDIA_PROVIDER_ENV_KEY);
Review Comment:
Why do we choose `json` as a configuration option? Is it convenient for users?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065706079
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
this.id = id;
Review Comment:
No, it's an empty map by default. You can have a look at the generated RSSProtos 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064314693
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageManager.java:
##########
@@ -54,4 +56,11 @@ public interface StorageManager {
// todo: add an interface that check storage isHealthy
void checkAndClearLeakedShuffleData(Collection<String> appIds);
+
+ /**
+ * This is only valid for LocalStorageManager. For other StorageManager, such as HdfsStorageManager, an empty map
Review Comment:
todo: this comment should be 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064141395
##########
storage/src/main/java/org/apache/uniffle/storage/common/DefaultStorageTypeProvider.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.uniffle.storage.common;
+
+import org.apache.uniffle.common.storage.SingleStorageType;
+
+public class DefaultStorageTypeProvider implements StorageTypeProvider {
+ @Override
+ public SingleStorageType getStorageTypeFor(String baseDir) {
+ return SingleStorageType.HDD;
Review Comment:
Thanks for the information. I will verify this command's output when I got a linux env.
However this is only valid for host/vm deployment, Uniffle Shuffle Server will be and should be deployed to container environments, and the data dir might be mounted by container runtime, the info provided by the linux kernel might not be accurate.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064141944
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -94,19 +101,26 @@ public class LocalStorageManager extends SingleStorageManager {
// We must make sure the order of `storageBasePaths` and `localStorages` is same, or some unit test may be fail
CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.size());
AtomicInteger successCount = new AtomicInteger();
+ ServiceLoader<StorageTypeProvider> loader = ServiceLoader.load(StorageTypeProvider.class);
Review Comment:
I think one is not enough, especially when you need to provide your own provider logic.
Although, the current logic may not handle provider evaluation order properly. But we can modify that later.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064480588
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -94,19 +101,26 @@ public class LocalStorageManager extends SingleStorageManager {
// We must make sure the order of `storageBasePaths` and `localStorages` is same, or some unit test may be fail
CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.size());
AtomicInteger successCount = new AtomicInteger();
+ ServiceLoader<StorageTypeProvider> loader = ServiceLoader.load(StorageTypeProvider.class);
Review Comment:
> I think one is not enough, especially when you need to provide your own provider logic.
Could you help provide some examples about needing multiple providers?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#issuecomment-1378129676
> If we introduce extra configuration option, we should add the document about this feature to explain how to use this feature.
Done. Please take another look when you have time.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1063199866
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
Do we need 1M 5M 1H for failure number？
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064140173
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageStatus.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum StorageStatus {
+ UNKNOWN(0),
+ NORMAL(1),
+ UNHEALTHY(2),
+ OVERUSED(3);
+
+ private final byte val;
+
+ StorageStatus(int code) {
+ assert (code >= -1 && code < 256);
+ this.val = (byte) code;
+ }
+
+ public final byte getCode() {
+ return val;
+ }
+
+ public RssProtos.StorageInfo.StorageStatus toProto() {
+ switch (this) {
+ case UNKNOWN:
+ return RssProtos.StorageInfo.StorageStatus.STORAGE_STATUS_UNKNOWN;
+ case NORMAL:
+ return RssProtos.StorageInfo.StorageStatus.NORMAL;
+ case UNHEALTHY:
+ return RssProtos.StorageInfo.StorageStatus.UNHEALTHY;
+ case OVERUSED:
+ return RssProtos.StorageInfo.StorageStatus.OVERUSED;
+ default:
+ return RssProtos.StorageInfo.StorageStatus.UNRECOGNIZED;
Review Comment:
Same as the `StorageType`s `UNRECOGNIZED` and `UNKNOWN`.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#issuecomment-1374836326
> I think this is an initial PR to support better assignment, it's better to list all subtasks, and indicate the progress in the PR title, like [1/n] xxxx, [2/n] xxxxxx.
The initial intention for this PR is to support shuffle server with LOCAL HDD disks better. You can see the parent and sub-issues from https://github.com/apache/incubator-uniffle/issues/448.
However this pr extends its scope a bit. I may open another issue to track this and related issues for better shuffle server assignments.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1063043839
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
@jerqi @zuston Hi, I have uploaded a new version per the design doc.
If you have no other objections, this PR is back to 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#issuecomment-1377088317
If we introduce extra configuration option, we should add the document about this feature to explain how to use this feature.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065698404
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
this.id = id;
Review Comment:
> Yes. the `storageInfoMap` should be an empty Map when deserializing from old code.
Could the default value be null?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#issuecomment-1375163478
PLAT @zuston .
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065509318
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageMediaFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.StorageMedia;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageMediaProvider;
+
+public class StorageMediaFromEnvProvider implements StorageMediaProvider {
+ private static Logger logger = LoggerFactory.getLogger(StorageMediaFromEnvProvider.class);
+ private Map<String, StorageMedia> localStorageTypes = Maps.newHashMap();
+
+ /**
+ * Returns current storage type for baseDir.
+ *
+ * @param baseDir the storage dir to check for
+ * @return LocalStorageType if valid or queryable. Unknown would be returned if it cannot be determined.
+ */
+ @Override
+ public StorageMedia getStorageMediaFor(String baseDir) {
+ File f = new File(baseDir);
+ while (f != null) {
+ if (localStorageTypes.containsKey(f.getPath())) {
+ return localStorageTypes.get(f.getPath());
+ }
+ f = f.getParentFile();
+ }
+ return StorageMedia.UNKNOWN;
+ }
+
+ @Override
+ public void init(RssConf conf) {
+ String envKey = conf.get(ShuffleServerConf.STORAGE_MEDIA_PROVIDER_ENV_KEY);
Review Comment:
The conf option are passed by container platforms.. Mostly are passed by json value in env.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: WIP: shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1058123170
##########
common/src/main/java/org/apache/uniffle/common/storage/LocalStorageInfoUtils.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.uniffle.common.storage;
+
+import com.google.common.collect.Maps;
+import org.apache.uniffle.proto.RssProtos;
+
+import java.util.Map;
+
+public class LocalStorageInfoUtils {
+ public static Map<String, RssProtos.LocalStorageInfo> toProto(
+ Map<String, LocalStorageInfo> info) {
+ Map<String, RssProtos.LocalStorageInfo> result = Maps.newHashMapWithExpectedSize(info.size());
+ info.forEach((k, v) -> result.put(k, v.toProto()));
+ return result;
+ }
+
+ public static Map<String, LocalStorageInfo> fromProto(Map<String, RssProtos.LocalStorageInfo> info) {
+ Map<String, LocalStorageInfo> result = Maps.newHashMapWithExpectedSize(info.size());
+ for (Map.Entry<String, RssProtos.LocalStorageInfo> entry : info.entrySet()) {
+ String key = entry.getKey();
+ RssProtos.LocalStorageInfo val = entry.getValue();
+ LocalStorageInfo localStorageInfo = new LocalStorageInfo(
+ val.getMountPoint(),
+ LocalStorageType.fromProto(val.getStorageType()),
+ val.getCapacity(),
+ val.getUsedBytes(),
+ LocalStorageStatus.fromProto(val.getStatus()));
+ }
Review Comment:
ah, thanks, forget to put into map.
I will add more UTs in next commit. You can start reviewing when it's ready.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1058689645
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
Such the speed of writing remoteStorageInfo..
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
Such as the speed of writing remoteStorageInfo..
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1060339327
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
Maybe we should collect remote storage, too. The failure times of writing is also meaningful. So we should give a more comprehensive design.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1060506122
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -336,6 +336,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription("Threshold when flushing shuffle data to persistent storage, recommend value would be 256K, "
+ "512K, or even 1M");
+ public static final ConfigOption<String> STORAGE_TYPE_PROVIDER_ENV_KEY = ConfigOptions
Review Comment:
admin might change it. I'm not sure it falls into the internal config catalog?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1058303978
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
Could we give this a more general abstraction? Could we unify the LocalStorageInfo and the RemoteStorageInfo.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1063155422
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
I left some comments in the doc.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064139369
##########
common/src/main/java/org/apache/uniffle/common/storage/SingleStorageType.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public enum SingleStorageType {
+ Unknown(0),
+ HDD(1),
+ SSD(2),
+ HDFS(3),
+ OBJECT(4);
Review Comment:
OK, let's be more specific here.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064480991
##########
server/src/main/java/org/apache/uniffle/server/storage/StorageTypeFromEnvProvider.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.uniffle.server.storage;
+
+import java.io.File;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.storage.SingleStorageType;
+import org.apache.uniffle.server.ShuffleServerConf;
+import org.apache.uniffle.storage.common.StorageTypeProvider;
+
+public class StorageTypeFromEnvProvider implements StorageTypeProvider {
Review Comment:
Got it. Make sense.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065371013
##########
common/pom.xml:
##########
@@ -83,6 +83,12 @@
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
+ <exclusions>
Review Comment:
Why do we need this?
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageInfo.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.uniffle.common.storage;
+
+import java.util.Objects;
+
+import org.apache.uniffle.proto.RssProtos;
+
+public class StorageInfo {
+ private String mountPoint;
+ private StorageMedia type;
+ private long capacity;
+ private long usedBytes;
+ // -1 indicates these field is not used and shall not be serialized to proto.
+ private long writingSpeed1M;
+ private long writingSpeed5M;
+ private long writingSpeed1H;
+ private long numberOfWritingFailures;
+ private StorageStatus status;
+
+ public StorageInfo(
+ String mountPoint,
+ StorageMedia type,
+ long capacity,
+ long usedBytes,
+ StorageStatus status) {
+ this.mountPoint = mountPoint;
+ this.type = type;
+ this.capacity = capacity;
+ this.usedBytes = usedBytes;
+ this.writingSpeed1M = -1;
+ this.writingSpeed5M = -1;
+ this.writingSpeed1H = -1;
+ this.numberOfWritingFailures = -1;
+ this.status = status;
+ }
+
+ public StorageInfo(
+ String mountPoint,
+ StorageMedia type,
+ long capacity,
+ long usedBytes,
+ long writingSpeed1M,
+ long writingSpeed5M,
+ long writingSpeed1H,
+ long numberOfWritingFailures,
+ StorageStatus status) {
+ this.mountPoint = mountPoint;
+ this.type = type;
+ this.capacity = capacity;
+ this.usedBytes = usedBytes;
+ this.writingSpeed1M = writingSpeed1M;
+ this.writingSpeed5M = writingSpeed5M;
+ this.writingSpeed1H = writingSpeed1H;
+ this.numberOfWritingFailures = numberOfWritingFailures;
+ this.status = status;
+ }
+
+
+ public RssProtos.StorageInfo toProto() {
+ RssProtos.StorageInfo.Builder builder = RssProtos.StorageInfo.newBuilder()
+ .setMountPoint(mountPoint)
+ .setStorageMedia(type.toProto())
+ .setCapacity(capacity)
+ .setUsedBytes(usedBytes)
+ .setStatus(status.toProto());
+ if (writingSpeed1M >= 0) {
+ builder.setWritingSpeed1M(writingSpeed1M);
+ builder.setWritingSpeed5M(writingSpeed5M);
+ builder.setWritingSpeed1H(writingSpeed1H);
+ }
+
+ if (numberOfWritingFailures >= 0) {
+ builder.setNumOfWritingFailures(numberOfWritingFailures);
+ }
+
+ return builder.build();
+ }
+
+ public StorageStatus getStatus() {
+ return status;
+ }
+
+ public StorageMedia getType() {
+ return type;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ StorageInfo that = (StorageInfo) o;
+ return Objects.equals(mountPoint, that.mountPoint) && type == that.type
+ && capacity == that.capacity
+ && usedBytes == that.usedBytes
+ && status == that.status;
+ }
+
+ @Override
+ public int hashCode() {
Review Comment:
Em....
`hasCode` method isn't consistent with `equal` method.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065680198
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -94,19 +101,26 @@ public class LocalStorageManager extends SingleStorageManager {
// We must make sure the order of `storageBasePaths` and `localStorages` is same, or some unit test may be fail
CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.size());
AtomicInteger successCount = new AtomicInteger();
+ ServiceLoader<StorageTypeProvider> loader = ServiceLoader.load(StorageTypeProvider.class);
Review Comment:
what if the `DefaultStorageMediaProvider` cannot provide accurate result?
The whole point of SPI is to provide an extend way to add plugins. So, the service loader must be a list?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065719091
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
Review Comment:
Should we convert from `RssProtos.StorageInfo` to `StorageInfo` in the `RssProtos.StorageInfo`?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [Feature] shuffle server report storage capability
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1060357332
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
why not directly using `repeated LocalStorageInfo localStorages`
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -336,6 +336,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription("Threshold when flushing shuffle data to persistent storage, recommend value would be 256K, "
+ "512K, or even 1M");
+ public static final ConfigOption<String> STORAGE_TYPE_PROVIDER_ENV_KEY = ConfigOptions
Review Comment:
Is this an internal config entry? If yes, could you mark it, rename from `STORAGE_TYPE_PROVIDER_ENV_KEY` to `__INTERNAL_STORAGE_TYPE_PROVIDER_ENV_KEY` ? WDYT
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064314426
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -336,6 +336,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription("Threshold when flushing shuffle data to persistent storage, recommend value would be 256K, "
+ "512K, or even 1M");
+ public static final ConfigOption<String> STORAGE_TYPE_PROVIDER_ENV_KEY = ConfigOptions
Review Comment:
do you have other suggestions?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1063196909
##########
proto/src/main/proto/Rss.proto:
##########
@@ -239,6 +239,7 @@ message ShuffleServerHeartBeatRequest {
int32 eventNumInFlush = 5;
repeated string tags = 6;
google.protobuf.BoolValue isHealthy = 7;
+ map<string, LocalStorageInfo> localStorages = 21; // mount point to local storage info mapping.
Review Comment:
> I left some comments in the doc.
I only saw one comment about default value for enum message, which has been addressed.
Is there any other comments?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064450593
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
> POJO is the concept of Java bean. I don't understand why we introduce this concept.
POJO is a not Java Bean, although it's always compared to Java Bean. The main reason to add suffix here is to avoid name conflicting, and the wrapper class for pb message falls into the POJO category.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1064744485
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
`storageMedia` ?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065528837
##########
common/src/main/java/org/apache/uniffle/common/storage/StorageTypePOJO.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.uniffle.common.storage;
+
+import org.apache.uniffle.proto.RssProtos.StorageInfo;
+
+/**
+ * The POJO stands for plain old java object. The suffix exists to avoid conflict with another StorageType class.
+ */
+public enum StorageTypePOJO {
Review Comment:
`StorageMedia` is ok for me.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #449: [ISSUE-448][Feature] shuffle server report storage info
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #449:
URL: https://github.com/apache/incubator-uniffle/pull/449#discussion_r1065719091
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -44,6 +51,21 @@ public ServerNode(
int eventNumInFlush,
Set<String> tags,
boolean isHealthy) {
+ this(id, ip, port, usedMemory, preAllocatedMemory, availableMemory, eventNumInFlush, tags, isHealthy,
+ Maps.newHashMap());
+ }
+
+ public ServerNode(
+ String id,
+ String ip,
+ int port,
+ long usedMemory,
+ long preAllocatedMemory,
+ long availableMemory,
+ int eventNumInFlush,
+ Set<String> tags,
+ boolean isHealthy,
+ Map<String, RssProtos.StorageInfo> storageInfoMap) {
Review Comment:
Should we convert from `RssProtos.StorageInfo` to `StorageInfo` in the `CoordinatorGrpcService`?
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org