You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "advancedxy (via GitHub)" <gi...@apache.org> on 2023/02/07 14:25:50 UTC

[GitHub] [incubator-uniffle] advancedxy opened a new pull request, #560: [#559] refactor: use system stubs to mutable environment variables

advancedxy opened a new pull request, #560:
URL: https://github.com/apache/incubator-uniffle/pull/560

   ### What changes were proposed in this pull request?
   refactor `RssUtils.setEnv` with EnvironmentVariables system stub
   
   ### Why are the changes needed?
   Better code quality. This fixes #559 
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   ### How was this patch tested?
   Existed 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] advancedxy commented on a diff in pull request #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1098839195


##########
common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java:
##########
@@ -43,7 +46,16 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
-public class RssUtilsTest {
+@ExtendWith(SystemStubsExtension.class)
+public class RssUtilsTest implements EnvironmentVariablesSetter {

Review Comment:
   `EnvTestBase` is defined in storage package, so it cannot be extended by `RssUtilsTest`.
   
   Why `EnvTestBase` is defined in storage rather than common package? 
   Because maven doesn't support transitive  test dependency[^1], which means if we put `EnvTestBase` in common, every other modules' pom.xml would redeclare the test-jar dependency, which is tedious. Something like below:
   
   ```
       <dependency>
         <groupId>org.apache.uniffle</groupId>
         <artifactId>rss-common</artifactId>
         <type>test-jar</type>
         <scope>test</scope>
       </dependency>
   ```
   
   [^1]: https://issues.apache.org/jira/browse/MNG-1378



-- 
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 #560: [#559] test: use withEnvironmentVariable to replace RssUtilsTest#setEnv

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#issuecomment-1422043572

   Merged, thanks @kaijchen 


-- 
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] kaijchen commented on a diff in pull request #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1099573027


##########
server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java:
##########
@@ -20,22 +20,16 @@
 import java.io.File;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import uk.org.webcompere.systemstubs.environment.EnvironmentVariables;
-import uk.org.webcompere.systemstubs.jupiter.SystemStub;
-import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;
 
 import org.apache.uniffle.common.util.ByteUnit;
+import org.apache.uniffle.storage.EnvTestBase;

Review Comment:
   New test writers may not be aware of this `EnvTestBase`, because it's hiding in the `storage` package.
   And I think this kind of abstraction isn't saving us much work.



-- 
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 merged pull request #560: [#559] test: use withEnvironmentVariable to replace RssUtilsTest#setEnv

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy merged PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560


-- 
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 #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1098839706


##########
storage/src/test/java/org/apache/uniffle/storage/EnvTestBase.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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;

Review Comment:
   Answered in the previous comment.



-- 
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] kaijchen commented on a diff in pull request #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1098810106


##########
storage/src/test/java/org/apache/uniffle/storage/EnvTestBase.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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;

Review Comment:
   Why is this class in `storage` package?



##########
common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java:
##########
@@ -43,7 +46,16 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
-public class RssUtilsTest {
+@ExtendWith(SystemStubsExtension.class)
+public class RssUtilsTest implements EnvironmentVariablesSetter {

Review Comment:
   Could this class extend `EnvTestBase`?



-- 
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 #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#issuecomment-1420909722

   # [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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 [#560](https://codecov.io/gh/apache/incubator-uniffle/pull/560?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (10c0793) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/5cba4e77618ebb0d7dd057ca6e23513c33d8a918?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5cba4e7) will **increase** coverage by `0.80%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #560      +/-   ##
   ============================================
   + Coverage     60.87%   61.67%   +0.80%     
   + Complexity     1796     1663     -133     
   ============================================
     Files           213      200      -13     
     Lines         12372    11096    -1276     
     Branches       1050      928     -122     
   ============================================
   - Hits           7531     6844     -687     
   + Misses         4434     3874     -560     
   + Partials        407      378      -29     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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/server/storage/LocalStorageManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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% <0.00%> (-1.13%)` | :arrow_down: |
   | [...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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) | | |
   | [...java/org/apache/hadoop/mapred/SortWriteBuffer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1NvcnRXcml0ZUJ1ZmZlci5qYXZh) | | |
   | [...preduce/task/reduce/RssRemoteMergeManagerImpl.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc1JlbW90ZU1lcmdlTWFuYWdlckltcGwuamF2YQ==) | | |
   | [...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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==) | | |
   | [...pache/hadoop/mapreduce/task/reduce/RssShuffle.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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==) | | |
   | [...g/apache/hadoop/mapred/SortWriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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==) | | |
   | [...pache/hadoop/mapreduce/task/reduce/RssFetcher.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0ZldGNoZXIuamF2YQ==) | | |
   | [.../java/org/apache/hadoop/mapreduce/RssMRConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SQ29uZmlnLmphdmE=) | | |
   | [...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SVXRpbHMuamF2YQ==) | | |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-uniffle/pull/560?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] advancedxy commented on a diff in pull request #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1099608283


##########
server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java:
##########
@@ -20,22 +20,16 @@
 import java.io.File;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import uk.org.webcompere.systemstubs.environment.EnvironmentVariables;
-import uk.org.webcompere.systemstubs.jupiter.SystemStub;
-import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;
 
 import org.apache.uniffle.common.util.ByteUnit;
+import org.apache.uniffle.storage.EnvTestBase;

Review Comment:
   > New test writers may not be aware of this `EnvTestBase`, because it's hiding in the `storage` package.
   
   I don't agree. `HDFSTestBase` is used almost everywhere(which has already extends `EnvTestBase`).   It's not a problem of visibility.
   
   > And I think this kind of abstraction isn't saving us much work.
   
   This is a fair point. Did review all the code in the pr, thought annotate all the test classes. I prefer current approach.
   



-- 
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] kaijchen commented on a diff in pull request #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1098857999


##########
common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java:
##########
@@ -43,7 +46,16 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
-public class RssUtilsTest {
+@ExtendWith(SystemStubsExtension.class)
+public class RssUtilsTest implements EnvironmentVariablesSetter {

Review Comment:
   Can we just declear `@SystemStub private static EnvironmentVariables env;` at every test requires it. And reduce the coupling?



-- 
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] kaijchen commented on a diff in pull request #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1098859178


##########
server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java:
##########
@@ -20,22 +20,16 @@
 import java.io.File;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import uk.org.webcompere.systemstubs.environment.EnvironmentVariables;
-import uk.org.webcompere.systemstubs.jupiter.SystemStub;
-import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;
 
 import org.apache.uniffle.common.util.ByteUnit;
+import org.apache.uniffle.storage.EnvTestBase;

Review Comment:
   It looks weird to extend a `storage` test from `server` package.



-- 
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 #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1098874842


##########
server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java:
##########
@@ -20,22 +20,16 @@
 import java.io.File;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import uk.org.webcompere.systemstubs.environment.EnvironmentVariables;
-import uk.org.webcompere.systemstubs.jupiter.SystemStub;
-import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;
 
 import org.apache.uniffle.common.util.ByteUnit;
+import org.apache.uniffle.storage.EnvTestBase;

Review Comment:
   Let me think about it. 
   But to be honest，storage is essentially depended by all the other packages except common.



-- 
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 #560: [#559] refactor: use system stubs to mutable environment variables

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1099654563


##########
server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java:
##########
@@ -20,22 +20,16 @@
 import java.io.File;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import uk.org.webcompere.systemstubs.environment.EnvironmentVariables;
-import uk.org.webcompere.systemstubs.jupiter.SystemStub;
-import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;
 
 import org.apache.uniffle.common.util.ByteUnit;
+import org.apache.uniffle.storage.EnvTestBase;

Review Comment:
   Found another way to archive this functionality. 
   You concern should be addressed.



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