Skip to content

[BEAM-775] Remove Aggregators from PipelineResults and Examples in Java SDK#2184

Closed
pabloem wants to merge 3 commits intoapache:masterfrom
pabloem:remove-agg-java
Closed

[BEAM-775] Remove Aggregators from PipelineResults and Examples in Java SDK#2184
pabloem wants to merge 3 commits intoapache:masterfrom
pabloem:remove-agg-java

Conversation

@pabloem
Copy link
Member

@pabloem pabloem commented Mar 7, 2017

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.

@pabloem
Copy link
Member Author

pabloem commented Mar 7, 2017

r: @bjchambers
I've removed Aggregators from the examples and PipelineResults. I plan to work on removing them from the internal piping in the SDK now.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 69.923% when pulling 41296b3 on pabloem:remove-agg-java into b45381d on apache:master.

@asfbot
Copy link

asfbot commented Mar 7, 2017

@pabloem pabloem closed this Mar 8, 2017
@pabloem pabloem reopened this Mar 8, 2017
@pabloem
Copy link
Member Author

pabloem commented Mar 8, 2017

Relaunching tests with PR close/reopen.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 69.923% when pulling 41296b3 on pabloem:remove-agg-java into b45381d on apache:master.

@asfbot
Copy link

asfbot commented Mar 8, 2017

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

Copy link
Contributor

Choose a reason for hiding this comment

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

live this as "unmatchedWords"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}

@Test
public void testGetAggregatorValuesWhenClientThrowsExceptionThrowsAggregatorRetrievalException()
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to keep first refactor the aggregator querying code for use to query metrics, and just convert these tests to metrics?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean keep getAggregatorValues functions, and tests? We may leave as-is and have another PR (right away : )) to add metrics querying and tests. What do you think about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

PR 2223 adds querying for metrics.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 70.041% when pulling 7b7ab3b on pabloem:remove-agg-java into b45381d on apache:master.

@asfbot
Copy link

asfbot commented Mar 9, 2017

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

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 69.989% when pulling 5eecd36 on pabloem:remove-agg-java into 3f0fe91 on apache:master.

@asfbot
Copy link

asfbot commented Mar 18, 2017

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

@pabloem
Copy link
Member Author

pabloem commented Mar 20, 2017

@bjchambers can we get this in and I'll do independent PRs for the other runners?

@davorbonaci
Copy link
Member

@pabloem, is there something blocking this PR? If so, it would be good to have a list of those things given that this is blocking the first stable release?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 70.53% when pulling f603d60 on pabloem:remove-agg-java into 8302783 on apache:master.

@pabloem pabloem force-pushed the remove-agg-java branch 2 times, most recently from 7275266 to a0d1175 Compare April 18, 2017 22:45
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 70.349% when pulling a0d1175 on pabloem:remove-agg-java into eb2e6aa on apache:master.

@pabloem
Copy link
Member Author

pabloem commented Apr 19, 2017

@bjchambers can we move forward with this? I can remove the latest commit (Removing Aggregators from PipelineResults and subclasses.) and make that change later, but removing from tests and examples should be okay no?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 70.11% when pulling 1c4ccc1 on pabloem:remove-agg-java into 6d3fe51 on apache:master.

@pabloem pabloem closed this Apr 19, 2017
@pabloem pabloem reopened this Apr 19, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 70.0% when pulling 0f75d28 on pabloem:remove-agg-java into 8319369 on apache:master.

@dhalperi
Copy link
Contributor

Fyi, you'll have to rebase this PR to resolve a conflict looks like.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 69.996% when pulling ba1466b on pabloem:remove-agg-java into b47fd52 on apache:master.

@bjchambers
Copy link
Contributor

Please rebase/squash/clean-up commit message?

@pabloem
Copy link
Member Author

pabloem commented Apr 24, 2017

Done. Just need to wait for tests.

@pabloem pabloem force-pushed the remove-agg-java branch 2 times, most recently from 79da0eb to 714f2a2 Compare April 24, 2017 23:35
@pabloem
Copy link
Member Author

pabloem commented Apr 25, 2017

After running mvn clean verify there's nullpointer failures in the Apex runner. I believe these were already happenning?

@bjchambers
Copy link
Contributor

bjchambers commented Apr 25, 2017 via email

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 69.731% when pulling 74c2664 on pabloem:remove-agg-java into 7e6f1b7 on apache:master.

@asfgit asfgit closed this in 4fabaef Apr 25, 2017
@pabloem pabloem deleted the remove-agg-java branch April 26, 2017 03:08
@zorro786
Copy link

zorro786 commented Jun 8, 2018

@pabloem I was using Aggregator to keep counter across each InputT bundles. What should be used now? Does Counter work in my case? If so what should be the implementing class of it?

@pabloem
Copy link
Member Author

pabloem commented Jun 8, 2018 via email

@zorro786
Copy link

zorro786 commented Jun 9, 2018 via email

@pabloem
Copy link
Member Author

pabloem commented Jun 9, 2018 via email

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.

8 participants

Comments