You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "zuston (via GitHub)" <gi...@apache.org> on 2023/03/21 10:16:46 UTC
[GitHub] [incubator-uniffle] zuston opened a new pull request, #749: [#731] Make blockId layout configured by client side
zuston opened a new pull request, #749:
URL: https://github.com/apache/incubator-uniffle/pull/749
<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
Examples:
- "[#123] feat(operator): support xxx"
- "[#233] fix: check null before access result in xxx"
- "[MINOR] refactor: fix typo in variable name"
- "[MINOR] docs: fix typo in README"
- "[#255] test: fix flaky test NameOfTheTest"
Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. Contributor guidelines:
https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md
3. If the PR is unfinished, please mark this PR as draft.
-->
### What changes were proposed in this pull request?
(Please outline the changes and how this PR fixes the issue.)
### Why are the changes needed?
(Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.)
Fix: # (issue)
### Does this PR introduce _any_ user-facing change?
(Please list the user-facing changes introduced by your change, including
1. Change in user-facing APIs.
2. Addition or removal of property keys.)
No.
### How was this patch tested?
(Please test your changes, and provide instructions on how to test it:
1. If you add a feature or fix a bug, add a test to cover your changes.
2. If you fix a flaky test, repeat it for many times to prove it works.)
--
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 pull request #749: [#731] Make blockId layout configured by client side
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1590898520
> Any movement on this particular feature?
No, do you want to pick up this?
--
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 #749: [#731] Make blockId layout configured by client side
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1495415433
We need reconsider this pr. Task can cycle and layout can be configured. We should think more.
--
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 closed pull request #749: [#731] Make blockId layout configured by client side
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston closed pull request #749: [#731] Make blockId layout configured by client side
URL: https://github.com/apache/incubator-uniffle/pull/749
--
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 pull request #749: [#731] Make blockId layout configured by client side
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1486665815
How about discussing this in weekly meeting?
--
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 #749: [#731] Make blockId layout configured by client side
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1486476892
I'm not sure about this change. It gives too much freedom to the user side, and I'm worried about the forward and backward compatibilities.
Would you mind to post a design doc or improvement proposal here to illustrate your idea more clear, especially about the compatibility issues 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
Re: [PR] [#731] Make blockId layout configured by client side [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1865649820
> The reason this is possible is that the block ids are bound to shuffle ids, it shouldn't interfere with each other.
But we also need to get all valid blockIds by the upstream map Ids in client.
--
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] connorlwilkes commented on pull request #749: [#731] Make blockId layout configured by client side
Posted by "connorlwilkes (via GitHub)" <gi...@apache.org>.
connorlwilkes commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1590795514
Any movement on this particular 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] zuston commented on pull request #749: [#731] Make blockId layout configured by client side
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1482417941
Could you help review this @jerqi
--
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 #749: [#731] Make blockId layout configured by client side
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1486547111
It's dangerous that let users configure the length directly. Maybe we could provide several options for them, such as `BIG_JOB`, `LONG_TIME_JOB`, etc.
--
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] xianjingfeng commented on a diff in pull request #749: [#731] Make blockId layout configured by client side
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#discussion_r1149958015
##########
common/src/main/java/org/apache/uniffle/common/BlockIdLayoutConfig.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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;
+
+import java.util.Map;
+
+import org.apache.uniffle.common.config.RssClientConf;
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.util.Constants;
+
+public class BlockIdLayoutConfig {
+ // BlockId is long and consist of partitionId, taskAttemptId, atomicInt
+ // the length of them are ATOMIC_INT_MAX_LENGTH + PARTITION_ID_MAX_LENGTH + TASK_ATTEMPT_ID_MAX_LENGTH = 63
+ private static final int BLOCK_ID_LENGTH = 63;
+
+ /**
+ * The keys are configured in the conf of BLOCKID_LAYOUT
+ */
+ public static final String PARTITION_ID_LENGTH = "partitionIdLength";
+ public static final String TASK_ATTEMPT_ID_LENGTH = "taskAttemptIdLength";
+ public static final String SEQUENCE_ID_LENGTH = "sequenceIdLength";
+
+ private int partitionIdLength;
+ private int taskAttemptIdLength;
+ private int sequenceIdLength;
+
+ private BlockIdLayoutConfig(int partitionIdLength, int taskAttemptIdLength, int sequenceIdLength) {
+ this.partitionIdLength = partitionIdLength;
+ this.taskAttemptIdLength = taskAttemptIdLength;
+ this.sequenceIdLength = sequenceIdLength;
+ }
+
+ public int getPartitionIdLength() {
+ return partitionIdLength;
+ }
+
+ public int getTaskAttemptIdLength() {
+ return taskAttemptIdLength;
+ }
+
+ public int getSequenceIdLength() {
+ return sequenceIdLength;
+ }
+
+ @Override
+ public String toString() {
+ return "BlockIdLayoutConfig{"
+ + "partitionIdLength=" + partitionIdLength
+ + ", taskAttemptIdLength=" + taskAttemptIdLength
+ + ", sequenceIdLength=" + sequenceIdLength
+ + '}';
+ }
+
+ public static boolean validate(Map<String, String> blockIdLayoutMap) {
+ try {
+ from(blockIdLayoutMap);
+ return true;
+ } catch (Exception e) {
+ return false;
+ }
+ }
+
+ public static BlockIdLayoutConfig from(RssConf rssConf) {
+ Map<String, String> blockIdLayoutMap = rssConf.get(RssClientConf.BLOCKID_LAYOUT);
+ if (blockIdLayoutMap == null || blockIdLayoutMap.isEmpty()) {
+ return from();
+ }
+
+ return from(blockIdLayoutMap);
+ }
+
+ public static BlockIdLayoutConfig from() {
+ return new BlockIdLayoutConfig(
+ Constants.PARTITION_ID_MAX_LENGTH,
+ Constants.TASK_ATTEMPT_ID_MAX_LENGTH,
+ Constants.ATOMIC_INT_MAX_LENGTH
+ );
+ }
+
+ public static BlockIdLayoutConfig from(int partitionIdLength, int taskAttemptIdLength, int sequenceIdLength) {
+ if (partitionIdLength + taskAttemptIdLength + sequenceIdLength != BLOCK_ID_LENGTH) {
Review Comment:
And should we put this judge logic into the construtor of BlockIdLayoutConfig?
--
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 pull request #749: [#731] Make blockId layout configured by client side
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1486481888
> Would you mind to post a design doc or improvement proposal here to illustrate your idea more clear, especially about the compatibility issues here.
I think the reason of this changed has been exploded in #731 description. 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] codecov-commenter commented on pull request #749: [#731] Make blockId layout configured by client side
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1478830523
## [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/749?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 [#749](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6b8f1c1) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/2cb22ff5869fadf59f6013357de0827dc12f215d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2cb22ff) will **increase** coverage by `2.07%`.
> The diff coverage is `50.40%`.
```diff
@@ Coverage Diff @@
## master #749 +/- ##
============================================
+ Coverage 60.65% 62.72% +2.07%
- Complexity 1897 1915 +18
============================================
Files 238 226 -12
Lines 13012 11155 -1857
Branches 1091 1100 +9
============================================
- Hits 7892 6997 -895
+ Misses 4682 3781 -901
+ Partials 438 377 -61
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL01SSWRIZWxwZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...pache/hadoop/mapreduce/task/reduce/RssShuffle.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc1NodWZmbGUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3YyL2FwcC9Sc3NNUkFwcE1hc3Rlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...he/uniffle/client/impl/ShuffleWriteClientImpl.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL1NodWZmbGVXcml0ZUNsaWVudEltcGwuamF2YQ==) | `33.41% <0.00%> (-0.25%)` | :arrow_down: |
| [...client/request/CreateShuffleReadClientRequest.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9yZXF1ZXN0L0NyZWF0ZVNodWZmbGVSZWFkQ2xpZW50UmVxdWVzdC5qYXZh) | `0.00% <ø> (ø)` | |
| [.../java/org/apache/uniffle/common/util/IdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi91dGlsL0lkSGVscGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...pache/uniffle/server/ShuffleServerGrpcService.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyR3JwY1NlcnZpY2UuamF2YQ==) | `0.79% <0.00%> (-0.02%)` | :arrow_down: |
| [...org/apache/uniffle/common/BlockIdLayoutConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9CbG9ja0lkTGF5b3V0Q29uZmlnLmphdmE=) | `17.02% <17.02%> (ø)` | |
| [.../org/apache/uniffle/common/config/ConfigUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9jb25maWcvQ29uZmlnVXRpbHMuamF2YQ==) | `92.38% <73.33%> (-3.18%)` | :arrow_down: |
| [...g/apache/hadoop/mapred/SortWriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1NvcnRXcml0ZUJ1ZmZlck1hbmFnZXIuamF2YQ==) | `80.10% <100.00%> (+0.21%)` | :arrow_up: |
| ... and [9 more](https://codecov.io/gh/apache/incubator-uniffle/pull/749?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [16 files with indirect coverage changes](https://codecov.io/gh/apache/incubator-uniffle/pull/749/indirect-changes?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] xianjingfeng commented on a diff in pull request #749: [#731] Make blockId layout configured by client side
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#discussion_r1149956456
##########
common/src/main/java/org/apache/uniffle/common/BlockIdLayoutConfig.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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;
+
+import java.util.Map;
+
+import org.apache.uniffle.common.config.RssClientConf;
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.util.Constants;
+
+public class BlockIdLayoutConfig {
+ // BlockId is long and consist of partitionId, taskAttemptId, atomicInt
+ // the length of them are ATOMIC_INT_MAX_LENGTH + PARTITION_ID_MAX_LENGTH + TASK_ATTEMPT_ID_MAX_LENGTH = 63
+ private static final int BLOCK_ID_LENGTH = 63;
+
+ /**
+ * The keys are configured in the conf of BLOCKID_LAYOUT
+ */
+ public static final String PARTITION_ID_LENGTH = "partitionIdLength";
+ public static final String TASK_ATTEMPT_ID_LENGTH = "taskAttemptIdLength";
+ public static final String SEQUENCE_ID_LENGTH = "sequenceIdLength";
+
+ private int partitionIdLength;
+ private int taskAttemptIdLength;
+ private int sequenceIdLength;
+
+ private BlockIdLayoutConfig(int partitionIdLength, int taskAttemptIdLength, int sequenceIdLength) {
+ this.partitionIdLength = partitionIdLength;
+ this.taskAttemptIdLength = taskAttemptIdLength;
+ this.sequenceIdLength = sequenceIdLength;
+ }
+
+ public int getPartitionIdLength() {
+ return partitionIdLength;
+ }
+
+ public int getTaskAttemptIdLength() {
+ return taskAttemptIdLength;
+ }
+
+ public int getSequenceIdLength() {
+ return sequenceIdLength;
+ }
+
+ @Override
+ public String toString() {
+ return "BlockIdLayoutConfig{"
+ + "partitionIdLength=" + partitionIdLength
+ + ", taskAttemptIdLength=" + taskAttemptIdLength
+ + ", sequenceIdLength=" + sequenceIdLength
+ + '}';
+ }
+
+ public static boolean validate(Map<String, String> blockIdLayoutMap) {
+ try {
+ from(blockIdLayoutMap);
+ return true;
+ } catch (Exception e) {
+ return false;
+ }
+ }
+
+ public static BlockIdLayoutConfig from(RssConf rssConf) {
+ Map<String, String> blockIdLayoutMap = rssConf.get(RssClientConf.BLOCKID_LAYOUT);
+ if (blockIdLayoutMap == null || blockIdLayoutMap.isEmpty()) {
+ return from();
+ }
+
+ return from(blockIdLayoutMap);
+ }
+
+ public static BlockIdLayoutConfig from() {
+ return new BlockIdLayoutConfig(
+ Constants.PARTITION_ID_MAX_LENGTH,
+ Constants.TASK_ATTEMPT_ID_MAX_LENGTH,
+ Constants.ATOMIC_INT_MAX_LENGTH
+ );
+ }
+
+ public static BlockIdLayoutConfig from(int partitionIdLength, int taskAttemptIdLength, int sequenceIdLength) {
+ if (partitionIdLength + taskAttemptIdLength + sequenceIdLength != BLOCK_ID_LENGTH) {
Review Comment:
How about if one of them is 0?
--
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] xianjingfeng commented on a diff in pull request #749: [#731] Make blockId layout configured by client side
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#discussion_r1149959186
##########
common/src/main/java/org/apache/uniffle/common/BlockIdLayoutConfig.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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;
+
+import java.util.Map;
+
+import org.apache.uniffle.common.config.RssClientConf;
+import org.apache.uniffle.common.config.RssConf;
+import org.apache.uniffle.common.util.Constants;
+
+public class BlockIdLayoutConfig {
+ // BlockId is long and consist of partitionId, taskAttemptId, atomicInt
+ // the length of them are ATOMIC_INT_MAX_LENGTH + PARTITION_ID_MAX_LENGTH + TASK_ATTEMPT_ID_MAX_LENGTH = 63
+ private static final int BLOCK_ID_LENGTH = 63;
+
+ /**
+ * The keys are configured in the conf of BLOCKID_LAYOUT
+ */
+ public static final String PARTITION_ID_LENGTH = "partitionIdLength";
+ public static final String TASK_ATTEMPT_ID_LENGTH = "taskAttemptIdLength";
+ public static final String SEQUENCE_ID_LENGTH = "sequenceIdLength";
+
+ private int partitionIdLength;
+ private int taskAttemptIdLength;
+ private int sequenceIdLength;
+
+ private BlockIdLayoutConfig(int partitionIdLength, int taskAttemptIdLength, int sequenceIdLength) {
+ this.partitionIdLength = partitionIdLength;
+ this.taskAttemptIdLength = taskAttemptIdLength;
+ this.sequenceIdLength = sequenceIdLength;
+ }
+
+ public int getPartitionIdLength() {
+ return partitionIdLength;
+ }
+
+ public int getTaskAttemptIdLength() {
+ return taskAttemptIdLength;
+ }
+
+ public int getSequenceIdLength() {
+ return sequenceIdLength;
+ }
+
+ @Override
+ public String toString() {
+ return "BlockIdLayoutConfig{"
+ + "partitionIdLength=" + partitionIdLength
+ + ", taskAttemptIdLength=" + taskAttemptIdLength
+ + ", sequenceIdLength=" + sequenceIdLength
+ + '}';
+ }
+
+ public static boolean validate(Map<String, String> blockIdLayoutMap) {
+ try {
+ from(blockIdLayoutMap);
+ return true;
+ } catch (Exception e) {
+ return false;
+ }
+ }
+
+ public static BlockIdLayoutConfig from(RssConf rssConf) {
+ Map<String, String> blockIdLayoutMap = rssConf.get(RssClientConf.BLOCKID_LAYOUT);
+ if (blockIdLayoutMap == null || blockIdLayoutMap.isEmpty()) {
+ return from();
+ }
+
+ return from(blockIdLayoutMap);
+ }
+
+ public static BlockIdLayoutConfig from() {
+ return new BlockIdLayoutConfig(
+ Constants.PARTITION_ID_MAX_LENGTH,
+ Constants.TASK_ATTEMPT_ID_MAX_LENGTH,
+ Constants.ATOMIC_INT_MAX_LENGTH
+ );
+ }
+
+ public static BlockIdLayoutConfig from(int partitionIdLength, int taskAttemptIdLength, int sequenceIdLength) {
+ if (partitionIdLength + taskAttemptIdLength + sequenceIdLength != BLOCK_ID_LENGTH) {
+ throw new IllegalArgumentException("The sum of all parts' length should be " + BLOCK_ID_LENGTH);
+ }
+ return new BlockIdLayoutConfig(partitionIdLength, taskAttemptIdLength, sequenceIdLength);
+ }
+
+ private static BlockIdLayoutConfig from(Map<String, String> blockIdLayoutMap) {
+ String partitionIdRawVal = blockIdLayoutMap.get(PARTITION_ID_LENGTH);
+ if (partitionIdRawVal == null) {
+ throw new IllegalArgumentException(PARTITION_ID_LENGTH + " must be configured.");
+ }
+ String taskAttemptIdRawVal = blockIdLayoutMap.get(TASK_ATTEMPT_ID_LENGTH);
+ if (taskAttemptIdRawVal == null) {
+ throw new IllegalArgumentException(TASK_ATTEMPT_ID_LENGTH + " must be configured.");
Review Comment:
How about use `assert`?
--
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 #749: [#731] Make blockId layout configured by client side
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1486540366
> > Would you mind to post a design doc or improvement proposal here to illustrate your idea more clear, especially about the compatibility issues here.
>
> I think the reason of this changed has been exploded in #731 description. WDYT? The detailed design is to be compatible with different apps.
From #731, I get the need to expand task attempt id. But rather than to increase the length of task_attempt_id or make it configurable, there might be other ways to solve this issue. For example, you can roll over the the task_attempt_id to zero and start again once it exceeds the capacity as long as the number of one shuffle map tasks doesn't exceeds the maximum number of the TASK_ATTEMPT_ID_MAX_LENGTH. The reason this is possible is that the block ids are bound to shuffle ids, it shouldn't interfere with each other.
Also, I'm more concerned about the compatibility issues. To make the length of fields in block ids adaptive itself is appealing,
it's just that we really have to think it through and make sure it wouldn't break things.
--
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 pull request #749: [#731] Make blockId layout configured by client side
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #749:
URL: https://github.com/apache/incubator-uniffle/pull/749#issuecomment-1491162712
> It's dangerous that let users configure the length directly. Maybe we could provide several options for them, such as `BIG_JOB`, `LONG_TIME_JOB`, etc.
SGTM.
--
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