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.

Sign up for GitHub

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

Jump to bottom

Fixes #24 - Teach Table Generator how to write to a HDFS file. #25

Open

mcdan wants to merge 1 commit into Teradata:master from mcdan:mcdan/issues/24

mcdan commented Aug 30, 2017

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


          Fixes Teradata#24 - Teach Table Generator how to write to a HDFS file.

bccf2d8

 * 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

rschlussel-zz suggested changes

View reviewed changes

Contributor

rschlussel-zz left a 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"

src/main/java/com/teradata/tpcds/Session.java

                   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)
                   {
+                      this(scale, targetDirectory, suffix, table, nullString, separator, doNotTerminate, noSexism, parallelism,

Contributor

rschlussel-zz Sep 7, 2017

if the function arguments don't fit nicely on one line, can you put one per line?

this(
    scale,
    targetDirectory,
    ....)

src/main/java/com/teradata/tpcds/Session.java

+                              this.nullString,
+                              this.separator,
+                              this.doNotTerminate,
+                              noSexism,

Contributor

rschlussel-zz Sep 7, 2017

this.noSexism to match the rest of the arguments

src/main/java/com/teradata/tpcds/Session.java

@@ @@ -255,3 +310,38 @@ public String getCommandLineArguments() @@
                       return output.toString();
                   }
               }
+              class SerializableHadoopConfiguration implements Serializable

Contributor

rschlussel-zz Sep 7, 2017

can this be private?

src/main/java/com/teradata/tpcds/Session.java

@@ @@ -26,19 +30,20 @@ @@
               import static com.teradata.tpcds.Options.DEFAULT_SEPARATOR;
               import static com.teradata.tpcds.Options.DEFAULT_SUFFIX;
-              public class Session
+              public class Session implements Serializable

Contributor

rschlussel-zz Sep 7, 2017

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)

src/main/java/com/teradata/tpcds/Session.java

@@ @@ -255,3 +310,38 @@ public String getCommandLineArguments() @@
                       return output.toString();
                   }
               }
+              class SerializableHadoopConfiguration implements Serializable

Contributor

rschlussel-zz Sep 7, 2017

implements on a new line

src/main/java/com/teradata/tpcds/Session.java

                   }
                   public Table getOnlyTableToGenerate()
                   {
-                      if (!table.isPresent()) {
+                      if (!Optional.ofNullable(table).isPresent()) {

Contributor

rschlussel-zz Sep 7, 2017

same. Just check for null.

src/main/java/com/teradata/tpcds/Session.java

                           throw new TpcdsException("table not present");
                       }
-                      return table.get();
+                      return Optional.ofNullable(table).get();

Contributor

rschlussel-zz Sep 7, 2017

same

src/main/java/com/teradata/tpcds/Session.java

@@ @@ -225,8 +280,8 @@ public String getCommandLineArguments() @@
                       if (!suffix.equals(DEFAULT_SUFFIX)) {
                           output.append("--suffix ").append(suffix).append(" ");
                       }
-                      if (table.isPresent()) {
-                          output.append("--table ").append(table.get().getName()).append(" ");
+                      if (Optional.ofNullable(table).isPresent()) {

Contributor

rschlussel-zz Sep 7, 2017

just check for null

src/main/java/com/teradata/tpcds/Session.java

-                      if (table.isPresent()) {
-                          output.append("--table ").append(table.get().getName()).append(" ");
+                      if (Optional.ofNullable(table).isPresent()) {
+                          output.append("--table ").append(Optional.ofNullable(table).get().getName()).append(" ");

Contributor

rschlussel-zz Sep 7, 2017

You can just do table.getName()

src/main/java/com/teradata/tpcds/Session.java

                       this.scaling = new Scaling(scale);
                       this.targetDirectory = targetDirectory;
                       this.suffix = suffix;
-                      this.table = table;
+                      this.table = table.isPresent() ? table.get() : null;

Contributor

rschlussel-zz Sep 7, 2017

while you're here, it would be great to add requireNonNull() checks for the fields that shouldn't be null.

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

Labels

None yet