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

feature/remove-inline-accounting #6171

Merged
merged 2 commits into from
Apr 2, 2021
Merged

Conversation

jrouzierinverse
Copy link
Member

Description

Remove inline_accounting table.

NEWS file entries

Enhancements

  • Remove unused table inline_accounting

Delete branch after merge

NO

Checklist

  • Document the feature
  • Add unit tests
  • Add acceptance tests (TestLink)

@jrouzierinverse jrouzierinverse added this to the PacketFence-10.3 milestone Mar 16, 2021
@extrafu
Copy link
Member

extrafu commented Mar 26, 2021

@jrouzierinverse rebase and @nqb / @julsemaan review and merge if fine

Copy link
Contributor

@nqb nqb left a comment

Choose a reason for hiding this comment

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

Missing files

There is some references to inline.accounting inside pf.conf.defaults and documentation.conf

Drop of DB table

I see you dropped inline_accounting table.
Tell me if I'm wrong but it means that we lost "bandwidth history" for nodes.
I'm not able to see if we lost an old history.

db/upgrade-X.X.X-X.Y.Z.sql Outdated Show resolved Hide resolved
@julsemaan
Copy link
Collaborator

Other than @nqb's comments, it looks fine to me

@extrafu
Copy link
Member

extrafu commented Mar 29, 2021

@jrouzierinverse Finish the work on that PR before tackling bugs.

@jrouzierinverse jrouzierinverse force-pushed the feature/remove-inline-accounting branch from 1892500 to 51daaed Compare March 29, 2021 19:56
@nqb
Copy link
Contributor

nqb commented Mar 30, 2021

@jrouzierinverse: there is still some references to inline.accounting inside pf.conf.defaults and documentation.conf

@nqb
Copy link
Contributor

nqb commented Mar 30, 2021

@jrouzierinverse, I did the removal for old config settings. Let me know if it's ok.

I just need an answer on "Drop of DB table" topic I mentioned and we should be ready to merge.

@nqb
Copy link
Contributor

nqb commented Apr 1, 2021

Bump @jrouzierinverse

@nqb
Copy link
Contributor

nqb commented Apr 2, 2021

Task inline_accounting_maintenance run each minute and mark inactive sessions based on:

  • $accounting_session_timeout (default value: 300 seconds)
  • one day history

Then task update bandwidth_balance column value in node table.

There is also some logical related to management of security event.

I didn't see any jobs that cleanup inline_accounting table so it means all data will be lost. It should impact only users that extract data from this table for external usage. I will merge this PR as-is and add a note in Upgrade guide.

@nqb nqb merged commit 7e1a1da into devel Apr 2, 2021
nqb added a commit that referenced this pull request Apr 2, 2021
@satkunas satkunas deleted the feature/remove-inline-accounting branch May 15, 2024 18:57
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.

None yet

4 participants