Skip to content

[BEAM-823, BEAM-923] Add localhost option for DatastoreIO#1293

Closed
vikkyrk wants to merge 4 commits intoapache:masterfrom
vikkyrk:ds_doc
Closed

[BEAM-823, BEAM-923] Add localhost option for DatastoreIO#1293
vikkyrk wants to merge 4 commits intoapache:masterfrom
vikkyrk:ds_doc

Conversation

@vikkyrk
Copy link
Contributor

@vikkyrk vikkyrk commented Nov 7, 2016

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

  • Verified with Datastore emulator locally

@vikkyrk
Copy link
Contributor Author

vikkyrk commented Nov 7, 2016

R: @dhalperi

@vikkyrk vikkyrk changed the title [BEAM-823, BEAM-923] [BEAM-823, BEAM-923] Add localhost option for DatastoreIO Nov 7, 2016
@vikkyrk
Copy link
Contributor Author

vikkyrk commented Nov 9, 2016

R: @davorbonaci since Dan is busy.

* all returned results will be read by a single Dataflow worker in order to ensure correct data.
* {@link com.google.datastore.v1.Query.Builder#setLimit(Int32Value)} or if the Query contains
* inequality filters like {@code GREATER_THAN, LESS_THAN} etc., then all returned results
* will be read by a single Dataflow worker in order to ensure correct data. Since data is read from
Copy link
Member

Choose a reason for hiding this comment

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

Please abstract Dataflow away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

abstract Builder setQuery(Query query);
abstract Builder setNamespace(String namespace);
abstract Builder setNumQuerySplits(int numQuerySplits);
abstract Builder setLocalhost(String hostPort);
Copy link
Member

Choose a reason for hiding this comment

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

why is hostPort a String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have used 'InetAddress' but to keep it in sync with the DatastoreOptions which uses a String, I kept it consistent.

.build();
}

public DatastoreV1.Read withLocalhost(String localhost) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what localhost parameter here means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments.

@dhalperi
Copy link
Contributor

Thanks @davorbonaci ;

R: -@dhalperi

@vikkyrk
Copy link
Contributor Author

vikkyrk commented Nov 18, 2016

R: @davorbonaci addressed comments.

@vikkyrk
Copy link
Contributor Author

vikkyrk commented Dec 2, 2016

This needs to be rebased after template is merged #1369

@asfbot
Copy link

asfbot commented Dec 2, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5470/
--none--

@dhalperi
Copy link
Contributor

Status?

@vikkyrk
Copy link
Contributor Author

vikkyrk commented Dec 16, 2016

This might have merge conflict with #1369, so planning to serialize them.

@asfbot
Copy link

asfbot commented Jan 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6520/
--none--

@asfgit asfgit closed this in 8ee3572 Feb 3, 2017
@dhalperi
Copy link
Contributor

dhalperi commented Feb 3, 2017

Kicked off postcommit: https://builds.apache.org/view/Beam/job/beam_PostCommit_Java_MavenInstall/2522/org.apache.beam$beam-sdks-java-io-google-cloud-platform/testReport/

and it passed (the GCP-IO ITs ran and passed, at least). So merging.

Thanks!

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.

4 participants