Skip to content

[BEAM-8108] Add Chicago Taxi Example running on Flink#10531

Merged
Ardagan merged 4 commits intoapache:masterfrom
kamilwu:chicago-flink
Jan 21, 2020
Merged

[BEAM-8108] Add Chicago Taxi Example running on Flink#10531
Ardagan merged 4 commits intoapache:masterfrom
kamilwu:chicago-flink

Conversation

@kamilwu
Copy link
Contributor

@kamilwu kamilwu commented Jan 8, 2020


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@kamilwu
Copy link
Contributor Author

kamilwu commented Jan 8, 2020

@kkucharc Could you trigger some tests for me, please?
First, I need a seed job to be run:
Run Seed Job

After that:
Run Chicago Taxi on Dataflow
Run Chicago Taxi on Flink

Thanks!

@kkucharc
Copy link
Contributor

kkucharc commented Jan 8, 2020

Run Seed Job

1 similar comment
@aromanenko-dev
Copy link
Contributor

Run Seed Job

@kkucharc
Copy link
Contributor

kkucharc commented Jan 9, 2020

Run Chicago Taxi on Dataflow

1 similar comment
@aromanenko-dev
Copy link
Contributor

Run Chicago Taxi on Dataflow

@kamilwu
Copy link
Contributor Author

kamilwu commented Jan 9, 2020

Thanks for your help @aromanenko-dev! I'm investigating the failure

@aromanenko-dev
Copy link
Contributor

@kamilwu Please, let me know if you will need any other help.

@kkucharc
Copy link
Contributor

kkucharc commented Jan 9, 2020

Run Seed Job

@kkucharc
Copy link
Contributor

kkucharc commented Jan 9, 2020

Run Chicago Taxi on Dataflow

@kkucharc
Copy link
Contributor

kkucharc commented Jan 9, 2020

Run Chicago Taxi on Flink

@kkucharc
Copy link
Contributor

retest this please

@kkucharc
Copy link
Contributor

Run Seed Job

@kkucharc
Copy link
Contributor

Run Chicago Taxi on Flink

@kkucharc
Copy link
Contributor

Run Chicago Taxi on Dataflow

@kamilwu
Copy link
Contributor Author

kamilwu commented Jan 10, 2020

R: @pabloem Can I ask you for a review?

chicagoTaxiJob(delegate)
}

CronJobBuilder.cronJob(
Copy link
Contributor

Choose a reason for hiding this comment

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

PostcommitJobBuilder already adds periodic cron job

void defineAutoPostCommitJob(name) {

Any reason to add extra one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cron job defined by PostcommitJobBuilder is executed every six hours, which is too frequent for our needs. The extra one overrides it with more desired setting (every 24 hours)

Copy link
Member

@pabloem pabloem left a comment

Choose a reason for hiding this comment

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

I've reviewed almost everything (it LGTM btw) besides the file .test-infra/jenkins/job_PostCommit_Python_Chicago_Taxi_Example_Flink.groovy . If @Ardagan thinks it looks good, then this is fine to move forward.
If not, I'll take a look tomorrow morning!
Thanks.

Copy link
Member

@pabloem pabloem left a comment

Choose a reason for hiding this comment

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

Just one comment about Python version. Other than that, LGTM.

def numberOfWorkers = 5

Docker publisher = new Docker(scope, LoadTestsBuilder.DOCKER_CONTAINER_REGISTRY)
publisher.publish(':sdks:python:container:py2:docker', 'python2.7_sdk')
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should be using a py3 container straight out

Copy link
Member

Choose a reason for hiding this comment

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

though maybe then we'd have to run everything in py3 (meaning the target :portable:py2:chicagoTaxiExample). LMK what you think.

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 ran the test in py3 (both SDK harness and the pipeline program) and there are some errors connected with dill package. What do you think about leaving it "as is" and creating a jira ticket to move chicago taxi to py3? We will have to do the migration anyway in the nearest future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pabloem
Copy link
Member

pabloem commented Jan 15, 2020

feel free to merge afterwards @Ardagan @kkucharc

@kkucharc
Copy link
Contributor

Run Seed Job

@kkucharc
Copy link
Contributor

Run Chicago Taxi on Flink

@Ardagan Ardagan merged commit 287b81c into apache:master Jan 21, 2020
@kamilwu kamilwu deleted the chicago-flink branch January 21, 2020 15:48
@kamilwu
Copy link
Contributor Author

kamilwu commented Jan 21, 2020

Thanks everyone!

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.

5 participants

Comments