Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADR for global message counter #2334

Merged
merged 5 commits into from
Sep 17, 2018
Merged

ADR for global message counter #2334

merged 5 commits into from
Sep 17, 2018

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Sep 13, 2018

I wrote this as an ADR, as I think it may be useful to begin to do that for more SDK API level things. (Though in hindsight, this was small enough that its really easy to have summarized it in an issue)

/cc @rigelrozanski @cwgoes @alexanderbez

This is based off our conversation in #2312


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@ValarDragon ValarDragon force-pushed the dev/global_msg_counter branch from 937cc15 to 090a702 Compare September 13, 2018 22:24
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

sik

## Decision

Create a global message counter field of type uint64.
The choice of uint64 is because it is only getting incremented by
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer us to be consistent and use an int64 here. Although isolated it may seem fine - I think this term will have a tendency to be used in arithmetic, which may cause bad decisions down the road of somebody using uint64 for arithmetic instead of int64.

@@ -0,0 +1,54 @@
# ADR 001: Global Message Counter
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! maybe we should even have this file in an ADR subdirectory within architecture? or it's probably fine for now - but at some point in the future we should fill the docs/architecture with all sorts or architecture related goodies not just ADRs

@ValarDragon ValarDragon changed the base branch from dev/ADR_template to develop September 14, 2018 02:04
@codecov
Copy link

codecov bot commented Sep 14, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@b5f8350). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #2334   +/-   ##
==========================================
  Coverage           ?   65.04%           
==========================================
  Files              ?      135           
  Lines              ?     8399           
  Branches           ?        0           
==========================================
  Hits               ?     5463           
  Misses             ?     2574           
  Partials           ?      362

@ValarDragon ValarDragon force-pushed the dev/global_msg_counter branch from e15187e to 60a7422 Compare September 14, 2018 02:15
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Makes sense to me -- concept ACK 👍

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK, I agree with the proposal and think we can start implementing it

IMHO an ADR is a bit overkill for this.

@cwgoes cwgoes merged commit b09e859 into develop Sep 17, 2018
@cwgoes cwgoes deleted the dev/global_msg_counter branch September 17, 2018 15:26
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