Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IR Compiler] fix error when creating json file #1743

Merged
merged 2 commits into from
Jun 23, 2022
Merged

[IR Compiler] fix error when creating json file #1743

merged 2 commits into from
Jun 23, 2022

Conversation

shirly121
Copy link
Collaborator

@shirly121 shirly121 commented Jun 21, 2022

What do these changes do?

  1. write json file to /tmp instead of the current directory to solve the problem of file privilege
  2. attach job id to each json file name to avoid conflicts in concurrent scenarios

Related issue number

Fixes #1729

… to /tmp instead of current to have access to
@@ -46,9 +46,12 @@
// represent ir plan as a chain of operators
public class IrPlan implements Closeable {
private static IrCoreLibrary irCoreLib = IrCoreLibrary.INSTANCE;
private static String PLAN_JSON_FILE = "plan.json";
// write to '/tmp' which is accessible from common users
private static String JSON_PLAN_DIR = "/tmp";
Copy link
Collaborator

@longbinlai longbinlai Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we obtain temporary folder from some environment variables. For example, Mac's temporary fold is not at "/tmp"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think /tmp is ok in macOS, which is a link to /private/tmp, Or maybe you can get the tmp directory by System.getProperty('java.io.tmpdir')

https://kodejava.org/how-do-i-get-operating-system-temporary-directory-folder/

package org.kodejava.lang;

public class TempDirExample {
    public static void main(String[] args) {
        // This is the property name for accessing OS temporary directory
        // or folder.
        String property = "java.io.tmpdir";

        // Get the temporary directory and print it.
        String tempDir = System.getProperty(property);
        System.out.println("OS temporary directory is " + tempDir);
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@codecov-commenter
Copy link

Codecov Report

Merging #1743 (88042b9) into main (7af26c9) will decrease coverage by 33.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1743       +/-   ##
===========================================
- Coverage   73.23%   40.22%   -33.01%     
===========================================
  Files          89       89               
  Lines        9764     9764               
===========================================
- Hits         7151     3928     -3223     
- Misses       2613     5836     +3223     
Impacted Files Coverage Δ
python/graphscope/tests/unittest/test_java_app.py 0.00% <0.00%> (-100.00%) ⬇️
...ython/graphscope/tests/unittest/test_cython_ast.py 0.00% <0.00%> (-100.00%) ⬇️
...ython/graphscope/tests/unittest/test_serailaize.py 0.00% <0.00%> (-100.00%) ⬇️
python/graphscope/analytical/udf/patch.py 3.47% <0.00%> (-96.53%) ⬇️
python/graphscope/tests/unittest/test_lazy.py 0.00% <0.00%> (-96.30%) ⬇️
...thon/graphscope/tests/unittest/test_scalability.py 0.00% <0.00%> (-96.16%) ⬇️
python/graphscope/tests/unittest/test_app.py 0.00% <0.00%> (-95.66%) ⬇️
...hon/graphscope/tests/unittest/test_create_graph.py 0.00% <0.00%> (-92.00%) ⬇️
python/graphscope/tests/unittest/test_graph.py 0.00% <0.00%> (-85.57%) ⬇️
python/graphscope/tests/unittest/test_context.py 0.00% <0.00%> (-81.04%) ⬇️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7af26c9...88042b9. Read the comment docs.

@shirly121 shirly121 merged commit 2cba970 into alibaba:main Jun 23, 2022
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.

[BUG] Panicked at 'Create json file': 'plan.json' error: Read-only file system
4 participants