Allow setting the checkpoint directory through SparkConnection #182
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This eliminates the need to set a new "conf_path" attribute on the configuration dictionary before returning it.
Instead of using this function to get the config and add attributes to it, we now separately get the config with load_conf_file() and pass attributes to Spark. I've translated some of the tests for load_conf() to tests for load_conf_file().
Previously we always set the checkpoint directory to be the same as spark.local.dir, which we call "tmp_dir". However, this doesn't make sense because tmp_dir should be on a disk local to each executor, and the checkpoint directory has to be on shared storage to work correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This work is for #181.
This PR adds a new
checkpoint_dirargument toSparkConnection. This is a required argument, so this is a breaking change.This fixes a bug where we always set Spark's checkpoint directory to
tmp_dir, which we used to set the spark.local.dir configuration option. The problem is that these directories should be on separate disks!tmp_dirshould be on a disk local to each executor. The checkpoint directory should be on shared storage so that all of the executors can access the same directory. (If you are running locally as thehlinkscript does, this distinction does not really matter.)While working on this, I have also removed the
hlink.scripts.main.load_conf()function, since it had some confusing logic in it. That logic has moved into the main script'scli()function and should hopefully be more straightforward now. Instead of setting the"conf_path"key in the return dictionary,hlink.configs.load_conf.load_conf_file()now returns a tuple of (file path, config dictionary).