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

Database support for Gauge #185

Merged
merged 48 commits into from Sep 13, 2016

Conversation

Projects
None yet
5 participants
@shivarammysore
Contributor

shivarammysore commented Aug 30, 2016

No description provided.

harshad91 and others added some commits Jul 8, 2016

Merge pull request #9 from harshad91/master
merging faucet database related changes
@shivarammysore

This comment has been minimized.

Show comment
Hide comment
@shivarammysore

shivarammysore Aug 30, 2016

Contributor

Rahul tested the entire setup to work correctly. @shivarammysore reviewed the architecture.

Contributor

shivarammysore commented Aug 30, 2016

Rahul tested the entire setup to work correctly. @shivarammysore reviewed the architecture.

@shivarammysore

This comment has been minimized.

Show comment
Hide comment
@shivarammysore

shivarammysore Aug 30, 2016

Contributor

This code was developed as a part of the SDNhackfest project (www.SDNHackfest.org).

Contributor

shivarammysore commented Aug 30, 2016

This code was developed as a part of the SDNhackfest project (www.SDNHackfest.org).

@KitL

This comment has been minimized.

Show comment
Hide comment
@KitL

KitL Aug 30, 2016

Member

Structurally I am concerned about how the code is being added to Gauge. Gauge should just throw updates at watchers, and watchers are where the code for handling pushing to the database should be. IE gauge shouldnt need to import anything to do with the database.

Also, you should build new watchers for use with couch db rather than modifying existing ones to also send stats to couch. At the moment this will crash in the REANNZ office because we are not using couchdb so when it tries to open its config file I'll get an Exception. You need to leave the existing watchers as they are.

Furthermore the config should come in through config.py. watcher.py shouldnt be parsing config. I would also suggest that you should just use the new config format, so your db config can all be included in gauge.yaml.

So structurally the only files you should really need to touch is watcher.py and config.py. There may also be some minor changes to how gauge.py handles adding and removing dps so that your watchers can monitor that effectively. But mostly I would expect to see that you would add a GaugeFlowTableCouchPoller class and maybe a GaugeDPCouchLogger.

I'll add inline comments to sorta clarify what I mean.

Member

KitL commented Aug 30, 2016

Structurally I am concerned about how the code is being added to Gauge. Gauge should just throw updates at watchers, and watchers are where the code for handling pushing to the database should be. IE gauge shouldnt need to import anything to do with the database.

Also, you should build new watchers for use with couch db rather than modifying existing ones to also send stats to couch. At the moment this will crash in the REANNZ office because we are not using couchdb so when it tries to open its config file I'll get an Exception. You need to leave the existing watchers as they are.

Furthermore the config should come in through config.py. watcher.py shouldnt be parsing config. I would also suggest that you should just use the new config format, so your db config can all be included in gauge.yaml.

So structurally the only files you should really need to touch is watcher.py and config.py. There may also be some minor changes to how gauge.py handles adding and removing dps so that your watchers can monitor that effectively. But mostly I would expect to see that you would add a GaugeFlowTableCouchPoller class and maybe a GaugeDPCouchLogger.

I'll add inline comments to sorta clarify what I mean.

@KitL

This comment has been minimized.

Show comment
Hide comment
@KitL

KitL Aug 30, 2016

Member

I hope that mostly makes sense.

Member

KitL commented Aug 30, 2016

I hope that mostly makes sense.

@shivarammysore

This comment has been minimized.

Show comment
Hide comment
@shivarammysore

shivarammysore Aug 30, 2016

Contributor

It would be better if the architecture and model is documented in
architecture.rst file so that it is clear.

Thanks

On Mon, Aug 29, 2016 at 7:54 PM, KitL notifications@github.com wrote:

Structurally I am concerned about how the code is being added to Gauge.
Gauge should just throw updates at watchers, and watchers are where the
code for handling pushing to the database should be. IE gauge shouldnt need
to import anything to do with the database.

Also, you should build new watchers for use with couch db rather than
modifying existing ones to also send stats to couch. At the moment this
will crash in the REANNZ office because we are not using couchdb so when it
tries to open its config file I'll get an Exception. You need to leave the
existing watchers as they are.

Furthermore the config should come in through config.py. watcher.py
shouldnt be parsing config. I would also suggest that you should just use
the new config format, so your db config can all be included in gauge.yaml.

So structurally the only files you should really need to touch is
watcher.py and config.py. There may also be some minor changes to how
gauge.py handles adding and removing dps so that your watchers can monitor
that effectively. But mostly I would expect to see that you would add a
GaugeFlowTableCouchPoller class and maybe a GaugeDPCouchLogger.

I'll add inline comments to sorta clarify what I mean.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#185 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABmf8_ElzyW5mCm2aktZVE0SovM6I--rks5qk5tsgaJpZM4JwEYe
.

Contributor

shivarammysore commented Aug 30, 2016

It would be better if the architecture and model is documented in
architecture.rst file so that it is clear.

Thanks

On Mon, Aug 29, 2016 at 7:54 PM, KitL notifications@github.com wrote:

Structurally I am concerned about how the code is being added to Gauge.
Gauge should just throw updates at watchers, and watchers are where the
code for handling pushing to the database should be. IE gauge shouldnt need
to import anything to do with the database.

Also, you should build new watchers for use with couch db rather than
modifying existing ones to also send stats to couch. At the moment this
will crash in the REANNZ office because we are not using couchdb so when it
tries to open its config file I'll get an Exception. You need to leave the
existing watchers as they are.

Furthermore the config should come in through config.py. watcher.py
shouldnt be parsing config. I would also suggest that you should just use
the new config format, so your db config can all be included in gauge.yaml.

So structurally the only files you should really need to touch is
watcher.py and config.py. There may also be some minor changes to how
gauge.py handles adding and removing dps so that your watchers can monitor
that effectively. But mostly I would expect to see that you would add a
GaugeFlowTableCouchPoller class and maybe a GaugeDPCouchLogger.

I'll add inline comments to sorta clarify what I mean.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#185 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABmf8_ElzyW5mCm2aktZVE0SovM6I--rks5qk5tsgaJpZM4JwEYe
.

@anarkiwi anarkiwi merged commit ea0b243 into faucetsdn:master Sep 13, 2016

anarkiwi added a commit that referenced this pull request Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment