Writing quickstart + test for Bigtable by billyjacobson · Pull Request #1431 · GoogleCloudPlatform/java-docs-samples

billyjacobson · 2019-05-21T21:11:28Z

No description provided.

lesv

Isn't there a pom.xml or something to build & test this? I hate to just check in something that can't be built on an automated basis.
@kurtisvg FYI

kurtisvg · 2019-05-21T22:16:17Z

Isn't there a pom.xml or something to build & test this

Looks like the pom here is being used.

billyjacobson · 2019-05-23T02:34:45Z

Isn't there a pom.xml or something to build & test this? I hate to just check in something that can't be built on an automated basis.
@kurtisvg FYI

Sorry, I put out the stuff with the pom in a separate PR cus that code was already existing, but this code is all new. You already reviewed that other PR though. Is there a way to trigger the tests to run for this submit?

kurtisvg · 2019-05-23T16:00:39Z

Oh the reason this isn't running tests is because it's not being merged to master. Do you want to merge them into the master branch?

lesv · 2019-05-24T19:17:08Z

I'm too til Tuesday

…

On Fri, May 24, 2019, 11:45 AM Billy Jacobson ***@***.***> wrote: @billyjacobson <https://github.com/billyjacobson> requested your review on: #1431 <#1431> Writing quickstart + test for Bigtable. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1431?email_source=notifications&email_token=AAIVLDTLP3JAEB3DGT5R5A3PXAZWNA5CNFSM4HOOYDS2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGORUGJPAQ#event-2366412674>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIVLDW3ZT25TNHA5Q65UPTPXAZWNANCNFSM4HOOYDSQ> .

…environment variable

kurtisvg

LGTM

kurtisvg · 2019-06-03T18:39:00Z

  public static void beforeClass() throws IOException {
-    projectId = System.getProperty(PROJECT_PROPERTY_NAME);
+    projectId = requireEnv("GOOGLE_CLOUD_PROJECT");
    if (projectId == null) {


requireEnv should throw an error if projectID is null, so you can remove this check

igorbernstein2 · 2019-06-07T20:54:50Z

+
+    } catch (Exception e) {
+      System.out.println("Error during functionName: \n" + e.toString());
+    }


Can we collapse the try-catch blocks into one?

try (BigtableDataClient dataClient = BigtableDataClient.create(settings)) { ... } catch (NotFoundException e) { System.err.println("Failed to read from a non-existent table: " + e.getMessage()); } catch (Exception e) { System.out.println("Error during functionName: \n" + e.toString()); }

igorbernstein2 · 2019-06-07T20:54:59Z

+      }
+
+    } catch (Exception e) {
+      System.out.println("Error during functionName: \n" + e.toString());


Whats functionName?

Also, please use System.err

igorbernstein2 · 2019-06-07T20:55:46Z


-  private static final String PROJECT_PROPERTY_NAME = "bigtable.project";
-  private static final String INSTANCE_PROPERTY_NAME = "bigtable.instance";
+  private static final String INSTANCE_PROPERTY_NAME = "BIGTABLE_TESTING_INSTANCE";


These shouldn't be called properties any more since its using env var now

igorbernstein2 · 2019-06-07T20:56:24Z

+    assertNotNull(
+        System.getenv(varName),
+        "System property '%s' is required to perform these tests.".format(varName));
+    return System.getenv(varName);


you can combine the return here:

return assertNotNull( System.getenv(varName), "System property '%s' is required to perform these tests.".format(varName));

assertNotNull doesn't return the value of System.getenv(varName)

Oops, I thought you were using Preconditions.checkNotNull, which does. Any reason not to use that?

igorbernstein2 · 2019-06-07T20:57:10Z

+    assertNotNull(
+        System.getenv(varName),
+        "System property '%s' is required to perform these tests.".format(varName));
+    return System.getenv(varName);


Same as above

igorbernstein2 · 2019-06-07T21:00:43Z

+public class QuickstartTest {
+
+  private static final String INSTANCE_PROPERTY_NAME = "BIGTABLE_TESTING_INSTANCE";
+  private static final String TABLE_ID = "quickstart-table";


Please don't hardcode the table id, it will cause these tests to be flaky when multiple PRs are opened

igorbernstein2 · 2019-06-07T21:01:14Z

+  }
+
+  @Before
+  public void setupStream() {


Please use the junit convention of calling this setUp()

igorbernstein2 · 2019-06-07T21:01:34Z

+    assertNotNull(
+        System.getenv(varName),
+        "System property '%s' is required to perform these tests.".format(varName));
+    return System.getenv(varName);


Same as above

igorbernstein2 · 2019-06-07T21:01:58Z

+
+public class Quickstart {
+
+  public static void quickstart(String projectId, String instanceId, String tableId) {


Please include a main()

billyjacobson added 3 commits May 21, 2019 13:40

Setup mvn package and copy in existing samples and tests

8cebdef

Fix style issues and ran tests

574063d

Writing quickstart + test for Bigtable

e1f37d1

billyjacobson requested a review from a team May 21, 2019 21:11

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2019

lesv reviewed May 21, 2019

View reviewed changes

billyjacobson and others added 5 commits May 22, 2019 16:26

Updating pom with current versions

c520973

Merge branch 'master' into bigtable-veneer-samples

ebe65b4

Adding default values for the system propertiesˆ

845c798

surefire instead of failsafe

dd82305

Merge branch 'bigtable-veneer-samples' into cbt-quickstart

5241730

billyjacobson changed the base branch from bigtable-veneer-samples to master May 23, 2019 16:48

billyjacobson and others added 5 commits May 23, 2019 12:48

Merge branch 'master' into cbt-quickstart

02b9963

setup table for testing using cbt tool, retriggering test

91fdca3

Removing unnecessary code from quickstart test that was causing failures

274ac80

Merge branch 'master' into cbt-quickstart

4b78a66

Merge branch 'master' into cbt-quickstart

961e25a

billyjacobson requested a review from lesv May 24, 2019 18:45

kurtisvg requested review from kurtisvg and removed request for lesv May 24, 2019 19:18

kurtisvg suggested changes May 24, 2019

View reviewed changes

billyjacobson and others added 2 commits May 28, 2019 17:01

cleaning up quickstart

ac3015a

Merge branch 'master' into cbt-quickstart

21f0f2a

kurtisvg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 30, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 30, 2019

Changing test variables to use GOOGLE_CLOUD_PROJECT and the instance …

d52b4a1

…environment variable

Merge branch 'master' into cbt-quickstart

ba1ec9c

billyjacobson requested a review from kurtisvg June 3, 2019 17:38

kurtisvg approved these changes Jun 3, 2019

View reviewed changes

billyjacobson merged commit 1a491c8 into master Jun 3, 2019

billyjacobson deleted the cbt-quickstart branch June 3, 2019 19:49

igorbernstein2 reviewed Jun 7, 2019

View reviewed changes


		public class Quickstart {

		public static void quickstart(String projectId, String instanceId, String tableId) {

Conversation

billyjacobson commented May 21, 2019

Uh oh!

lesv left a comment

Choose a reason for hiding this comment

Uh oh!

kurtisvg commented May 21, 2019

Uh oh!

billyjacobson commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtisvg commented May 23, 2019

Uh oh!

lesv commented May 24, 2019 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kurtisvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

billyjacobson commented May 23, 2019 •

edited

Loading