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

Source as a URI #123

Merged
merged 1 commit into from
Mar 29, 2018
Merged

Source as a URI #123

merged 1 commit into from
Mar 29, 2018

Conversation

duglin
Copy link
Collaborator

@duglin duglin commented Mar 23, 2018

In an attempt to get to a MVP for the spec, I'm proposing to collapse source-related attributes (source* & namespace) down to a single URI whose format/encoding is event producer defined.

Future PRs can suggest that we split out certain bits of data from this URI (e.g. for performance, optimization or other reasons), but then those discussions will be more focused on just the specific need being addressed, so hopefully will be easier to reach a consensus.

Signed-off-by: Doug Davis dug@us.ibm.com

Signed-off-by: Doug Davis <dug@us.ibm.com>
@yaronha
Copy link
Contributor

yaronha commented Mar 23, 2018

@dug LGTM

@SeanFeldman
Copy link

👍

@clemensv
Copy link
Contributor

I just closed #95 in support of this PR. This simplification does align with what we believe we need for addressing our customers' needs. We will have optimization comments subsequent to this PR.

@shalka
Copy link

shalka commented Mar 23, 2018

Agree with @clemensv .. My only (minor) concern would be that we should type 'source' as a string instead of a URI since we're consolidating namespace into it as well. The format (which is yet to be determined) may or may not take the form of a URI, but we can't make that distinction yet.

@duglin
Copy link
Collaborator Author

duglin commented Mar 23, 2018

I thought about that but I remembered that when I casually mentioned it as a string on a call a long time ago some folks pushed back, so I picked URI as the starting point. Would you be ok with discussing a possible re-typing in a follow-on PR?

@shalka
Copy link

shalka commented Mar 23, 2018

@duglin That's reasonable. I like the consolidation, so long as we agree that a follow-on PR may (or may not) change the format of 'source'.
:shipit:

Copy link

@mrutkows mrutkows left a comment

Choose a reason for hiding this comment

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

LGTM. Wherever we can avoid String generalizations for identifying (well-formed) URIs is a very good thing IMO.

@markpeek
Copy link
Contributor

LGTM

@deissnerk
Copy link
Contributor

LGTM
Under the assumption that it is not carved in stone, and that we are free to change this, after we learned from discussions and early implementations.

@inlined
Copy link
Contributor

inlined commented Mar 28, 2018

I don't want to let great be the enemy of good. I will ask in the future that we specify some subset of URI to build APIs on top of this (e.g. one that includes an authority), though URIs LGTM.

@ultrasaurus
Copy link
Contributor

LGTM

@ultrasaurus ultrasaurus mentioned this pull request Mar 29, 2018
@dklyle
Copy link
Contributor

dklyle commented Mar 29, 2018

LGTM

@duglin
Copy link
Collaborator Author

duglin commented Mar 29, 2018

Approved on 3/29 call.

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.