Skip to content

[BEAM-2724] Preparing support for Structured Names in Dataflow counters#3684

Closed
pabloem wants to merge 1 commit intoapache:masterfrom
pabloem:add-countername
Closed

[BEAM-2724] Preparing support for Structured Names in Dataflow counters#3684
pabloem wants to merge 1 commit intoapache:masterfrom
pabloem:add-countername

Conversation

@pabloem
Copy link
Copy Markdown
Member

@pabloem pabloem commented Aug 4, 2017

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.

  • Adding a CounterName class to help support structured names for counters in the future.
  • Also remove function calls from the apiclient.py file that are no longer used.
  • And also removing function calls used for aggregators, which have been dead code for a while.

@coveralls
Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling b1e15ab on pabloem:add-countername into ** on apache:master**.

@pabloem pabloem closed this Aug 8, 2017
@pabloem pabloem reopened this Aug 8, 2017
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 69.971% when pulling a4176f8 on pabloem:add-countername into 2fa4fde on apache:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.003%) to 69.975% when pulling a4176f8 on pabloem:add-countername into 2fa4fde on apache:master.

@pabloem
Copy link
Copy Markdown
Member Author

pabloem commented Aug 8, 2017

r: @aaltay
First change to add structured counter names in Dataflow runner. Adding the CounterName class.

self.system_name = system_name
self.output_index = output_index

def __hash__(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you need the hash function? What is the default one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The default hash function is related to the id() of the object. This means that without overriding it, this would be True:
hash(CounterName('arg1', 'arg2', 'arg3', 'arg4')) != hash(CounterName('arg1', 'arg2', 'arg3', 'arg4'))

I need counters to be indexable by their name. e.g.

my_d = {CounterName('arg1', 'arg2', 'arg3', 'arg4'): my_counter}
assert my_d[CounterName('arg1', 'arg2', 'arg3', 'arg4')] == my_counter

That's why I need to override the hash function. In fact, I'll add unit tests for this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

@aaltay
Copy link
Copy Markdown
Member

aaltay commented Aug 9, 2017

LGTM

@asfgit asfgit closed this in 2e17311 Aug 9, 2017
@pabloem pabloem deleted the add-countername branch August 10, 2017 16:35
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