You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/06/02 17:33:52 UTC
[GitHub] [beam] boyuanzz opened a new pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
boyuanzz opened a new pull request #11894:
URL: https://github.com/apache/beam/pull/11894
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-637786219
`test_pardo_timers_clear` doesn't work with multi-workers in fn_runner. Going to skip it with multi-workers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
ajamato commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-646358274
@robertwb Would you mind please taking a look at this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-637858581
Run Python Spark ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tweise commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
tweise commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r434087483
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py
##########
@@ -377,11 +377,6 @@ def process_timer(
assert_that(actual, equal_to(expected))
def test_pardo_timers_clear(self):
- if type(self).__name__ != 'FlinkRunnerTest':
Review comment:
As mentioned in the comment, `test_pardo_timers` and `test_pardo_timers_clear` could be merged. I had to fork it at that time to not lose the coverage for `fn_api_runner`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-637860496
> Maybe better to list the runners that should skip instead so that we don't limit coverage for new/unknown runners?
Re: @tweise It's kind of hard to do that since we have different configuration for the same runner. For example, `FnApiRunner` can run with multi-worker, or cached workers. Although it's not ideal to limit a test case to specific test suite.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r436068037
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py
##########
@@ -377,11 +377,6 @@ def process_timer(
assert_that(actual, equal_to(expected))
def test_pardo_timers_clear(self):
- if type(self).__name__ != 'FlinkRunnerTest':
Review comment:
We have problems if overriding subclass. The `PortableRunnerTest` doesn't support clearing timer, but its subclass `FlinkRunnerTest` and `SparkRunnerTest` support it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r435595283
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py
##########
@@ -377,11 +377,6 @@ def process_timer(
assert_that(actual, equal_to(expected))
def test_pardo_timers_clear(self):
- if type(self).__name__ != 'FlinkRunnerTest':
Review comment:
I think it would be cleaner to add an override in the subclasses than test on `__name__` here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tweise commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
tweise commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r434181735
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner_test.py
##########
@@ -241,7 +241,8 @@ def process(self, kv, index=beam.DoFn.StateParam(index_state_spec)):
equal_to(expected))
# Inherits all other tests from fn_api_runner_test.FnApiRunnerTest
-
+ def test_pardo_timers_clear(self):
Review comment:
This may also be used for Spark. Skip should probably only occur in leaf classes that strictly need it. Otherwise, we are almost back to what we had before ;-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r434177432
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner_test.py
##########
@@ -241,7 +241,8 @@ def process(self, kv, index=beam.DoFn.StateParam(index_state_spec)):
equal_to(expected))
# Inherits all other tests from fn_api_runner_test.FnApiRunnerTest
-
+ def test_pardo_timers_clear(self):
Review comment:
Ah yes. I thought `FlinkRunnerTest` is inherited from `FnRunnerTest`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tweise commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
tweise commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r434176084
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner_test.py
##########
@@ -241,7 +241,8 @@ def process(self, kv, index=beam.DoFn.StateParam(index_state_spec)):
equal_to(expected))
# Inherits all other tests from fn_api_runner_test.FnApiRunnerTest
-
+ def test_pardo_timers_clear(self):
Review comment:
Doesn't this turn off the test for `FlinkRunnerTest` and anything else that extends `PortableRunner`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r442614800
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py
##########
@@ -377,11 +377,6 @@ def process_timer(
assert_that(actual, equal_to(expected))
def test_pardo_timers_clear(self):
- if type(self).__name__ != 'FlinkRunnerTest':
Review comment:
They are passing now. It seems like because of my FnApiRunner changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz merged pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz merged pull request #11894:
URL: https://github.com/apache/beam/pull/11894
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-637731199
Run Python PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on a change in pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #11894:
URL: https://github.com/apache/beam/pull/11894#discussion_r438299065
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py
##########
@@ -377,11 +377,6 @@ def process_timer(
assert_that(actual, equal_to(expected))
def test_pardo_timers_clear(self):
- if type(self).__name__ != 'FlinkRunnerTest':
Review comment:
I'm really surprised FnApiRunnerTest supports this but PortableRunnerTest does not. What error do we get?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-637921787
@robertwb Please take another look : ) A simple clear timer support is added to FnApiRunner. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-646874614
I'm going to merge this PR since the code that was requested to change has been removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #11894: [BEAM-7074] Enable test_pardo_timers_clear for fn_runner
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #11894:
URL: https://github.com/apache/beam/pull/11894#issuecomment-646725769
Run Python PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org