Default job name length for DistributedLzoIndexer #117
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
Truncates the job name to 200 characters by default and provides configuration options for overriding the name or length.
zman0900
changed the title
sjlee
reviewed
Sep 23, 2016
Also made method package-private and static for easier testing.
|
I added some tests and also ability to restore the old behavior by setting max length <= 0. |
sjlee
requested changes
Sep 24, 2016
| assertEquals(expected, job.getJobName()); | ||
| } | ||
|
|
||
| public void testCustomeName() throws Exception { |
There was a problem hiding this comment.
Typo: CustomeName -> CustomName
| static void setJobName(Job job, String[] args) { | ||
| final Configuration conf = job.getConfiguration(); | ||
|
|
||
| String name = conf.get(JOB_NAME_KEY, "Distributed Lzo Indexer " + Arrays.toString(args)); |
There was a problem hiding this comment.
Can we define a constant for "Distributed Lzo Indexer" so that it can be reused by the test?
| assertEquals(customName, job.getJobName()); | ||
| } | ||
|
|
||
| public void testCustomeNameTruncation() throws Exception { |
| assertEquals("12345...", job.getJobName()); | ||
| } | ||
|
|
||
| public void testDefaultLengthTruncation() throws Exception { |
There was a problem hiding this comment.
I don't really think it's important to test the default length. It should be sufficient between the custom name test and custom length test IMO. I'm OK with dropping this test. The fact that this test needs to rely on the default value (200) implicitly is also bit of a drawback (should the default change in the future).
sjlee
requested changes
Sep 24, 2016
| Job job = new Job(conf); | ||
| DistributedLzoIndexer.setJobName(job, args); | ||
|
|
||
| String expected = "Distributed Lzo Indexer [hdfs://cluster/user/test/..."; |
There was a problem hiding this comment.
We can use the constant here too.
| Job job = new Job(conf); | ||
| DistributedLzoIndexer.setJobName(job, args); | ||
|
|
||
| String expected = "Distributed Lzo Indexer [hdfs://cluster/user/test/output/file-m-00000.lzo, hdfs://cluster/user/test/output/file-m-00001.lzo, hdfs://cluster/user/test/output/file-m-00002.lzo, hdfs://cluster/user/test/output/file-m-00003.lzo, hdfs://cluster/user/test/output/file-m-00003.lzo]"; |
| String customName = "-<Custom Job Name>-"; | ||
|
|
||
| Configuration conf = new Configuration(false); | ||
| conf.set("lzo.indexer.distributed.job.name", customName); |
| }; | ||
|
|
||
| Configuration conf = new Configuration(false); | ||
| conf.set("lzo.indexer.distributed.job.name", "123456789"); |
There was a problem hiding this comment.
Let's use the constants in these 2 lines.
| }; | ||
|
|
||
| Configuration conf = new Configuration(false); | ||
| conf.setInt("lzo.indexer.distributed.job.name.max.length", 50); |
| }; | ||
|
|
||
| Configuration conf = new Configuration(false); | ||
| conf.setInt("lzo.indexer.distributed.job.name.max.length", 0); |
sjlee
approved these changes
Sep 24, 2016
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.
Truncates the job name to 200 characters by default and provides configuration options for overriding the name or length.
This fixes issue #25.