Skip to content

Conversation

@nevillelyh
Copy link
Contributor

@nevillelyh nevillelyh commented Apr 13, 2018

Rework of #1057

@nevillelyh nevillelyh force-pushed the andrisnoko/bigtable branch from 6192241 to 0be7b86 Compare April 13, 2018 17:03
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

class BigtableBulkWriter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add populateDisplayData where applicable.

final Iterable<KV<ByteString, Iterable<Mutation>>> elements = c.element();
final Iterator<KV<ByteString, Iterable<Mutation>>> iterator = elements.iterator();

while (iterator.hasNext()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you do foreach here you will save one line of code.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

class BigtableBulkWriter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are testing missing for this transform.


private final int numOfShards;

AssignToShard(int numOfShards) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

*/
class BigtableWriteException extends IOException {

public BigtableWriteException(KV<ByteString, Iterable<Mutation>> record, Throwable cause) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final to the parameters

@@ -0,0 +1,30 @@
/*
* Copyright 2017 Spotify AB.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018

@@ -0,0 +1,212 @@
/*
* Copyright 2017 Spotify AB.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018

@ravwojdyla
Copy link
Contributor

@andrisnoko ^

Copy link
Contributor

@ravwojdyla ravwojdyla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above.

@nevillelyh nevillelyh force-pushed the andrisnoko/bigtable branch from 445e5f8 to 834c0c3 Compare April 17, 2018 18:04
@codecov-io
Copy link

Codecov Report

Merging #1112 into master will increase coverage by 0.62%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1112      +/-   ##
==========================================
+ Coverage   81.82%   82.45%   +0.62%     
==========================================
  Files         119      119              
  Lines        4011     3983      -28     
  Branches      460      437      -23     
==========================================
+ Hits         3282     3284       +2     
+ Misses        729      699      -30
Impacted Files Coverage Δ
...main/scala/com/spotify/scio/bigtable/package.scala 72.85% <0%> (-9.41%) ⬇️
...spotify/scio/values/PairSCollectionFunctions.scala 96.52% <0%> (+1.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e30035f...834c0c3. Read the comment docs.

@andrisnoko
Copy link
Contributor

@ravwojdyla @nevillelyh

* Hepler to test createBulkShards.
*/
private static class TestPTransform extends
PTransform<PCollection<KV<ByteString, Iterable<Mutation>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 the formatting looks weird

@regadas regadas merged commit d3e8ee1 into master Apr 17, 2018
@regadas regadas deleted the andrisnoko/bigtable branch April 17, 2018 19:43
jto pushed a commit that referenced this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants