You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by terry xian <fx...@yahoo.com> on 2020/09/01 20:32:32 UTC
Could someone review my pull request 12695 ?
Hi,
My pull request [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and google_cloud_bigtable_client_core to 1.16.0 by terryxian78 · Pull Request #12695 · apache/beam was there for more than 3 days. Although I've added a reviewer (lukecwik), I am afraid that I missed something which might cause the PR not noticed (it is my first PR in Beam). I've asked some folks which work on spanner change review my change but need committee member for approval.
Could someone in committee review my PR?
Thanks!
|
|
|
| | |
|
|
|
| |
[BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and google_cloud_bigt...
Fixes https://issues.apache.org/jira/browse/BEAM-8758 R: @lukecwik CC: @suztomo The changes are: The main purpo...
|
|
|
Re: Could someone review my pull request 12695 ?
Posted by Niel Markwick <ni...@google.com>.
I have a bug/PR open on the spanner client libraries, but replacing
ImmutableList with List would be a breaking API change for them compared to
an already-released version, and so will be unlikely to be fixed soon
On Wed, 2 Sep 2020, 01:09 terry xian, <fx...@yahoo.com> wrote:
>
> As nielm pointed out in his comment of my PR, I added this because
>
> "spanner API change that exposes Guava classes is:
> googleapis/java-spanner/pull/81
> <https://github.com/googleapis/java-spanner/pull/81>,
>
> Specifically, adding AsyncResultSet in
> googleapis/java-spanner/pull/81/files#diff-7a9cb34faeb259be46b44f1878b7210f
>
> <https://github.com/googleapis/java-spanner/pull/81/files#diff-7a9cb34faeb259be46b44f1878b7210f>
> which returns an ImmutableList."
>
>
> Without this addition, the ApiSurface test would fail, please see: beam_PreCommit_Java_Commit
> #13264 test - testGcpApiSurface [Jenkins]
> <https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/13264/testReport/org.apache.beam.sdk.io.gcp/GcpApiSurfaceTest/testGcpApiSurface/>.
> So I was suggested to add new exposed class explicitly.
>
> beam_PreCommit_Java_Commit #13264 test - testGcpApiSurface [Jenkins]
>
>
> <https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/13264/testReport/org.apache.beam.sdk.io.gcp/GcpApiSurfaceTest/testGcpApiSurface/>
>
>
> Thanks!
>
>
>
>
> On Tuesday, September 1, 2020, 03:48:37 PM PDT, Chamikara Jayalath <
> chamikara@google.com> wrote:
>
>
> BTW this PR adds the following to the API surface.
>
> (com.google.common.collect.ImmutableCollection.class),
> (com.google.common.collect.ImmutableCollection.Builder.class),
> (com.google.common.collect.ImmutableList.class),
> (com.google.common.collect.ImmutableList.Builder.class),
> (com.google.common.collect.UnmodifiableIterator.class),
> (com.google.common.collect.UnmodifiableListIterator.class),
>
> Any objections to this ?
>
> Terry, could you explain the reason for adding this.
>
> Thanks,
> Cham
>
> On Tue, Sep 1, 2020 at 2:40 PM Chamikara Jayalath <ch...@google.com>
> wrote:
>
> LGTM. We can merge when tests pass.
>
> Thanks,
> Cham
>
> On Tue, Sep 1, 2020 at 1:32 PM terry xian <fx...@yahoo.com> wrote:
>
> Hi,
>
> My pull request [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and
> google_cloud_bigtable_client_core to 1.16.0 by terryxian78 · Pull Request
> #12695 · apache/beam <https://github.com/apache/beam/pull/12695> was
> there for more than 3 days. Although I've added a reviewer (lukecwik
> <https://github.com/lukecwik>), I am afraid that I missed something which
> might cause the PR not noticed (it is my first PR in Beam). I've asked some
> folks which work on spanner change review my change but need committee
> member for approval.
>
> Could someone in committee review my PR?
>
> Thanks!
>
>
> [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and google_cloud_bigt...
>
> Fixes https://issues.apache.org/jira/browse/BEAM-8758 R: @lukecwik CC:
> @suztomo The changes are: The main purpo...
> <https://github.com/apache/beam/pull/12695>
>
>
>
>
>
Re: Could someone review my pull request 12695 ?
Posted by terry xian <fx...@yahoo.com>.
As nielm pointed out in his comment of my PR, I added this because
"spanner API change that exposes Guava classes is:
googleapis/java-spanner/pull/81,
Specifically, adding AsyncResultSet in googleapis/java-spanner/pull/81/files#diff-7a9cb34faeb259be46b44f1878b7210f
which returns an ImmutableList."
Without this addition, the ApiSurface test would fail, please see: beam_PreCommit_Java_Commit #13264 test - testGcpApiSurface [Jenkins]. So I was suggested to add new exposed class explicitly.
|
|
| |
beam_PreCommit_Java_Commit #13264 test - testGcpApiSurface [Jenkins]
|
|
|
Thanks!
On Tuesday, September 1, 2020, 03:48:37 PM PDT, Chamikara Jayalath <ch...@google.com> wrote:
BTW this PR adds the following to the API surface.
(com.google.common.collect.ImmutableCollection.class),
(com.google.common.collect.ImmutableCollection.Builder.class),
(com.google.common.collect.ImmutableList.class),
(com.google.common.collect.ImmutableList.Builder.class),
(com.google.common.collect.UnmodifiableIterator.class),
(com.google.common.collect.UnmodifiableListIterator.class),
Any objections to this ?
Terry, could you explain the reason for adding this.
Thanks,Cham
On Tue, Sep 1, 2020 at 2:40 PM Chamikara Jayalath <ch...@google.com> wrote:
LGTM. We can merge when tests pass.
Thanks,Cham
On Tue, Sep 1, 2020 at 1:32 PM terry xian <fx...@yahoo.com> wrote:
Hi,
My pull request [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and google_cloud_bigtable_client_core to 1.16.0 by terryxian78 · Pull Request #12695 · apache/beam was there for more than 3 days. Although I've added a reviewer (lukecwik), I am afraid that I missed something which might cause the PR not noticed (it is my first PR in Beam). I've asked some folks which work on spanner change review my change but need committee member for approval.
Could someone in committee review my PR?
Thanks!
|
|
|
| | |
|
|
|
| |
[BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and google_cloud_bigt...
Fixes https://issues.apache.org/jira/browse/BEAM-8758 R: @lukecwik CC: @suztomo The changes are: The main purpo...
|
|
|
Re: Could someone review my pull request 12695 ?
Posted by Chamikara Jayalath <ch...@google.com>.
BTW this PR adds the following to the API surface.
(com.google.common.collect.ImmutableCollection.class),
(com.google.common.collect.ImmutableCollection.Builder.class),
(com.google.common.collect.ImmutableList.class),
(com.google.common.collect.ImmutableList.Builder.class),
(com.google.common.collect.UnmodifiableIterator.class),
(com.google.common.collect.UnmodifiableListIterator.class),
Any objections to this ?
Terry, could you explain the reason for adding this.
Thanks,
Cham
On Tue, Sep 1, 2020 at 2:40 PM Chamikara Jayalath <ch...@google.com>
wrote:
> LGTM. We can merge when tests pass.
>
> Thanks,
> Cham
>
> On Tue, Sep 1, 2020 at 1:32 PM terry xian <fx...@yahoo.com> wrote:
>
>> Hi,
>>
>> My pull request [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and
>> google_cloud_bigtable_client_core to 1.16.0 by terryxian78 · Pull Request
>> #12695 · apache/beam <https://github.com/apache/beam/pull/12695> was
>> there for more than 3 days. Although I've added a reviewer (lukecwik
>> <https://github.com/lukecwik>), I am afraid that I missed something
>> which might cause the PR not noticed (it is my first PR in Beam). I've
>> asked some folks which work on spanner change review my change but need
>> committee member for approval.
>>
>> Could someone in committee review my PR?
>>
>> Thanks!
>>
>>
>> [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and
>> google_cloud_bigt...
>>
>> Fixes https://issues.apache.org/jira/browse/BEAM-8758 R: @lukecwik CC:
>> @suztomo The changes are: The main purpo...
>> <https://github.com/apache/beam/pull/12695>
>>
>>
>>
>>
>>
Re: Could someone review my pull request 12695 ?
Posted by Chamikara Jayalath <ch...@google.com>.
LGTM. We can merge when tests pass.
Thanks,
Cham
On Tue, Sep 1, 2020 at 1:32 PM terry xian <fx...@yahoo.com> wrote:
> Hi,
>
> My pull request [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and
> google_cloud_bigtable_client_core to 1.16.0 by terryxian78 · Pull Request
> #12695 · apache/beam <https://github.com/apache/beam/pull/12695> was
> there for more than 3 days. Although I've added a reviewer (lukecwik
> <https://github.com/lukecwik>), I am afraid that I missed something which
> might cause the PR not noticed (it is my first PR in Beam). I've asked some
> folks which work on spanner change review my change but need committee
> member for approval.
>
> Could someone in committee review my PR?
>
> Thanks!
>
>
> [BEAM-8758] Google-cloud-spanner upgrade to 1.59.0 and google_cloud_bigt...
>
> Fixes https://issues.apache.org/jira/browse/BEAM-8758 R: @lukecwik CC:
> @suztomo The changes are: The main purpo...
> <https://github.com/apache/beam/pull/12695>
>
>
>
>
>