PHOENIX-7081 : Replace /tmp with {java.io.tmpdir} in tests #1808

Divneet18 · 2024-01-30T00:51:38Z

No description provided.

stoty · 2024-01-30T08:56:55Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java

@@ -89,6 +89,7 @@

 @Category(ParallelStatsDisabledTest.class)
 public class CreateTableIT extends ParallelStatsDisabledIT {
+    private static final String filePath = System.getProperty("java.io.tmpdir");


This name is not descriptive.
tmpPath would be better (in each file)

stoty · 2024-01-30T09:01:53Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java

@@ -1557,7 +1558,7 @@ public void testCoprocessorsForCreateIndexOnOldImplementation() throws Exception
            // Now roll back to the old indexing
            IndexUpgradeTool iut =
                    new IndexUpgradeTool(ROLLBACK_OP, tableName, null,
-                            "/tmp/index_upgrade_" + UUID.randomUUID().toString(), false, null,
+                            filePath + "/index_upgrade_" + UUID.randomUUID().toString(), false, null,


In the Hadoop Tools, path parameters are some times HDFS paths, so that is a case where simple "/tmp" is the right solution.
Those are also separate HDFS file systems, so there is no danger of coliision.

Have you checked that all of these instances are used as local paths, as opposed to HadoopFS paths ?

(I have checked this one, and this is correct, as this is a local FS path)

stoty · 2024-02-05T06:17:04Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/CsvBulkLoadToolIT.java

@@ -62,7 +62,7 @@ public class CsvBulkLoadToolIT extends BaseOwnClusterIT {

    private static Connection conn;
    private static String zkQuorum;
-    private static final String filePath = System.getProperty("java.io.tmpdir");


This is a HDFS path. not a local path.
This should NOT use java.io.tmpdir.

Please check each changed test to make sure that only local paths use java.io.tmpdir, and not HadoopFS paths.

stoty · 2024-02-07T05:52:32Z

phoenix-core-server/src/main/java/org/apache/phoenix/mapreduce/index/IndexUpgradeTool.java

@@ -739,7 +740,7 @@ private int startIndexRebuilds(HashMap<String, IndexInfo> indexInfos,
            String tenantId = indexInfo.getTenantId();
            String baseTable = indexInfo.getBaseTable();
            String schema = indexInfo.getSchemaName();
-            String outFile = "/tmp/index_rebuild_" +schema+"_"+ indexName +
+            String outFile = tmpPath + "/index_rebuild_" +schema+"_"+ indexName +


This one eventually becomes an HadoopFS path.

stoty

Please fix IndexUpgradeTool

stoty

Please verify EACH path instance, as I wrote above.

stoty · 2024-02-08T13:19:39Z

phoenix-core-server/src/main/java/org/apache/phoenix/mapreduce/AbstractBulkLoadTool.java

@@ -249,7 +250,7 @@ private int loadData(Configuration conf, CommandLine cmdLine) throws Exception {
        if (cmdLine.hasOption(OUTPUT_PATH_OPT.getOpt())) {
            outputPath = new Path(cmdLine.getOptionValue(OUTPUT_PATH_OPT.getOpt()));
        } else {
-            outputPath = new Path("/tmp/" + UUID.randomUUID());
+            outputPath = new Path(tmpPath + "/" + UUID.randomUUID());


Please check EACH PATH manually.
This is another obvious HadoopFS path, the type is org.apache.hadoop.fs.Path. It is defined just a few lines above.

stoty

Please revert the files that only have whitespace changes, so that it is immediately apparent which files were changed.

stoty

+1 LGTM.
I will address my nits on commit.

stoty · 2024-02-14T06:36:49Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java

@@ -167,9 +167,9 @@ public static synchronized void setup(
            //Maybe we're gonna have to refactor the namespace
            if(clusterInitialized) {
                tearDownMiniCluster(0);
-                System.setProperty("java.io.tmpdir", tmpDir);
+                System.setProperty("java.io.tmpdir", tmpPath);


The old variable name is better here.
This is not used a base for paths, it's only used for saving the value of the system property.

and the old variable name reflects this better.

stoty

Come to think of it, this is initaiizing the path variables too early.
MiniCluster DOES change "java.io.tmpdir" , and there is no guarantee that the miniCluster used for the tests is already running when the test is loaded.

We should take tmpdir value from after the miniCluster initialization, i.e. either

in a @before method
At the end of @BeforeClass (for @NeedsOwnCluster tests)
or in the test itself.

stoty · 2024-02-14T06:53:44Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java

@@ -87,9 +87,9 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest {
    private static final String [] TABLE_LIST_NAMESPACE_SIMPLIFIED = new String[1];

    private static final String [] TRANSACTIONAL_TABLE_LIST = new String[1];
-
+    private static String tmpPath = System.getProperty("java.io.tmpdir");


This is supposed to be value of tmpdir after minicluster has been started.
(But it is not, see my previous commenft()

stoty · 2024-02-14T07:03:47Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java

@@ -115,7 +114,7 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest {

    @BeforeClass
    public static synchronized void saveTmp () throws Exception {
-        tmpDir = System.getProperty("java.io.tmpdir");
+        tmpPath = System.getProperty("java.io.tmpdir");


This is the value of tmpDir before starting the minicluster.
It should only be used for restoring its value, and not be used in tests.

could you elaborate on this please since all i did here was change the variable name?

See the comment above.
the tmpdir values originally set, and re-set by minicluster must be kept separate.

Using the same tmpPath name we use elsewhere for constructing the test paths for saving the value leads to misunderstanding and errors like the one above.

You could keep the tmpDir name here, or use something like originalTmpDir.

stoty · 2024-02-27T05:10:15Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java

    private static String INPUT_LIST = "";
-    private static final String INPUT_FILE = "/tmp/input_file_index_upgrade.csv";
+    private static final String INPUT_FILE = tmpPath + "/input_file_index_upgrade.csv";


This is where you use the the saved tmpdir.

You should instead use tmpPath which is read AFTER minicluster is started, which can be a different value (depending onm HBase / Hadoop version)

stoty · 2024-02-27T05:12:36Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java

@@ -115,7 +114,7 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest {

    @BeforeClass
    public static synchronized void saveTmp () throws Exception {
-        tmpDir = System.getProperty("java.io.tmpdir");
+        tmpPath = System.getProperty("java.io.tmpdir");


See the comment above.
the tmpdir values originally set, and re-set by minicluster must be kept separate.

Using the same tmpPath name we use elsewhere for constructing the test paths for saving the value leads to misunderstanding and errors like the one above.

You could keep the tmpDir name here, or use something like originalTmpDir.

PHOENIX-7081 : Replace /tmp with {java.io.tmpdir} in tests

8c25364

stoty requested changes Jan 30, 2024

View reviewed changes

updated filePath to tmpPath

74eb70c

Divneet18 requested a review from stoty February 2, 2024 20:37

stoty requested changes Feb 5, 2024

View reviewed changes

removed tmpPaths for HDFS files

dde4f95

stoty reviewed Feb 7, 2024

View reviewed changes

stoty requested changes Feb 7, 2024

View reviewed changes

fixed checkstyle

d243889

stoty requested changes Feb 8, 2024

View reviewed changes

updated hdfs paths

867f71e

stoty requested changes Feb 9, 2024

View reviewed changes

reverting files to commit id b70f018:

cce9717

Divneet18 requested a review from stoty February 13, 2024 21:50

stoty approved these changes Feb 14, 2024

View reviewed changes

stoty requested changes Feb 14, 2024

View reviewed changes

stoty reviewed Feb 14, 2024

View reviewed changes

Divneet18 added 2 commits February 26, 2024 14:26

Updated based on nits

69a8ca2

removed import

964446f

stoty requested changes Feb 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-7081 : Replace /tmp with {java.io.tmpdir} in tests #1808

PHOENIX-7081 : Replace /tmp with {java.io.tmpdir} in tests #1808

Divneet18 commented Jan 30, 2024

stoty Jan 30, 2024

stoty Jan 30, 2024

stoty Feb 5, 2024

stoty Feb 7, 2024

stoty left a comment

stoty left a comment

stoty Feb 8, 2024

stoty left a comment

stoty left a comment

stoty Feb 14, 2024

stoty Feb 14, 2024

stoty left a comment

stoty Feb 14, 2024

stoty Feb 14, 2024

Divneet18 Feb 26, 2024

stoty Feb 27, 2024 •

edited

stoty Feb 27, 2024

stoty Feb 27, 2024 •

edited

PHOENIX-7081 : Replace /tmp with {java.io.tmpdir} in tests #1808

Are you sure you want to change the base?

PHOENIX-7081 : Replace /tmp with {java.io.tmpdir} in tests #1808

Conversation

Divneet18 commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty left a comment

Choose a reason for hiding this comment

stoty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty left a comment

Choose a reason for hiding this comment

stoty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty Feb 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty Feb 27, 2024 • edited

Choose a reason for hiding this comment

stoty Feb 27, 2024 •

edited

stoty Feb 27, 2024 •

edited