Fixes #24 - Teach Table Generator how to write to a HDFS file. #25
Conversation
* Make session & scaling serializable so it can be passed around * Teach session factory pattern to take in a hadoop config object * Make a serializable hadoop config object to ensure Session is serializable
There was a problem hiding this comment.
Thanks for the PR! Sorry it took me a while to get to it. Can you explain the motivation for making the session serializable? It seems unrelated to adding support for writing to hdfs.
If it is necessary, I think it would be good to put those changes in a separate commit.
Also, we generally don't include the issue number in the commit message title, so you can just say "Teach TableGenerator to write to an HDFS file"
| } | ||
|
|
||
| public Session(double scale, String targetDirectory, String suffix, Optional<Table> table, String nullString, char separator, boolean doNotTerminate, boolean noSexism, int parallelism, int chunkNumber, boolean overwrite) | ||
| { |
There was a problem hiding this comment.
if the function arguments don't fit nicely on one line, can you put one per line?
this(
scale,
targetDirectory,
....)
| Optional.ofNullable(this.table), | ||
| this.nullString, | ||
| this.separator, | ||
| this.doNotTerminate, |
There was a problem hiding this comment.
this.noSexism to match the rest of the arguments
| return output.toString(); | ||
| } | ||
| } | ||
|
|
| import static com.teradata.tpcds.Options.DEFAULT_SUFFIX; | ||
|
|
||
| public class Session | ||
| public class Session implements Serializable |
There was a problem hiding this comment.
implements should be on a newline to match the style of the rest of the project. (I thought that would be caught by checkstyle, but I guess not)
| return output.toString(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
implements on a new line
|
|
||
| public Table getOnlyTableToGenerate() | ||
| { | ||
| if (!table.isPresent()) { |
There was a problem hiding this comment.
same. Just check for null.
| if (!Optional.ofNullable(table).isPresent()) { | ||
| throw new TpcdsException("table not present"); | ||
| } | ||
| return table.get(); |
| output.append("--suffix ").append(suffix).append(" "); | ||
| } | ||
| if (table.isPresent()) { | ||
| output.append("--table ").append(table.get().getName()).append(" "); |
| } | ||
| if (table.isPresent()) { | ||
| output.append("--table ").append(table.get().getName()).append(" "); | ||
| if (Optional.ofNullable(table).isPresent()) { |
There was a problem hiding this comment.
You can just do table.getName()
| this.scaling = new Scaling(scale); | ||
| this.targetDirectory = targetDirectory; | ||
| this.suffix = suffix; | ||
| this.table = table; |
There was a problem hiding this comment.
while you're here, it would be great to add requireNonNull() checks for the fields that shouldn't be null.
serializable