Skip to content

Conversation

@Divneet18
Copy link
Contributor

No description provided.

@stoty
Copy link
Contributor

stoty commented Dec 14, 2023

Whilte this technically fixes the issue, it is not the ideal solution.
Most tests and the minicluster are already using java.io.tmpdir.

I would prefer to have all uses of the '/tmp' dir replaced with the value of java.io.tmpdir.
The tests set a unique java.io.tmpdir when starting the minicluster instance.

@stoty
Copy link
Contributor

stoty commented Dec 14, 2023

This PR also seems to have a lot of unrelated patches, I suggest rebasing on master and force pushing.

@Divneet18
Copy link
Contributor Author

@stoty i had some issues with my local git. i will create a new PR with fixed changes and will tag you.

@Divneet18 Divneet18 closed this Dec 14, 2023
@Divneet18 Divneet18 deleted the PHOENIX-7081 branch December 14, 2023 20:33
@stoty
Copy link
Contributor

stoty commented Dec 15, 2023

BTW no need to close, you can re-use your existing PRs, you just need to force-push the new patch form your original source branch.

@Divneet18
Copy link
Contributor Author

The tmp dir thats being generated for both tests are coming out to be the same, hence this strategy wont work. Shall we go ahead with the epoch strategy as done before?

@stoty
Copy link
Contributor

stoty commented Jan 9, 2024

Can you give me examples of those directories, @Divneet18 ?

If that is the case, then we should probably fix java.io.tmpdir in the maven setup.

@Divneet18
Copy link
Contributor Author

"/var/folders/bf/rk2sl26x6_52mztrsll5ywq00000gn/T/" was the directory that was generated for java.io.tmpdir for one of the tests i ran @stoty

@stoty
Copy link
Contributor

stoty commented Jan 10, 2024

Thanks.

I have created PHOENIX-7175 to set a unique tmpdir.
Are you interested in picking up that one @Divneet18 ? (otherwise I can do it)

@Divneet18
Copy link
Contributor Author

Yes I am working on it now :)

@Divneet18
Copy link
Contributor Author

Divneet18 commented Jan 11, 2024

@stoty i tried adding the property like you mentioned but still the folder names are the same for both. right now the folder shows up as "/Users/divneet.kaur/Desktop/phoenix/phoenix-core/target".

@stoty
Copy link
Contributor

stoty commented Jan 12, 2024

This is what we want.
If you run another test, it's going to be from a different directory, so the temp directories are gonna be be different, and we avoid the conflict.

ie, you run one test from ~/phoenix-master , and another from ~/phoenix-5.1

@Divneet18
Copy link
Contributor Author

@stoty i see makes sense. should i add the property change as a part of a separate PR for PHOENIX-7175 ? or can i add my changes to this PR only that I made for PHOENIX-7081

@stoty
Copy link
Contributor

stoty commented Jan 14, 2024

I have a slight preference for handling them in separate commits.

@stoty
Copy link
Contributor

stoty commented Jan 23, 2024

Looking at the code, we have a lot of similar cases.
We should expand the scope to fix all of those.
Search for "/tmp to see them.

Can you do those as well @Divneet18 , or should I take over ?

@Divneet18
Copy link
Contributor Author

Divneet18 commented Jan 23, 2024

@stoty I can do it. Seems like when i search for /tmp there are 50+ files that use it. I shouldnt change the .sh files right?
Just the files where the output directory is /tmp/..... ? I can do an initial commit and if theres more files i will do another commit?

@stoty
Copy link
Contributor

stoty commented Jan 23, 2024

Right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants