Skip to content
This repository was archived by the owner on Jun 27, 2020. It is now read-only.

Conversation

@R9295
Copy link
Contributor

@R9295 R9295 commented Jan 10, 2018

Implements and closes #46

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 289e654 on R9295:del-old-nodes into 9b2be9c on netjson:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

great work Aarnav!

Please just add the possibility to turn off (setting to False) and set this by default (we don't want to users upgrading to the latest version to find out about this new feature because they lost data).

Also add mention of this new setting in the README.

As usualy, new features must always be mentioned somewhere in the documentation because otherwise is as if they don't exist, keep this always in mind.

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 609382f on R9295:del-old-nodes into 9b2be9c on netjson:master.

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling cc79a54 on R9295:del-old-nodes into 9b2be9c on netjson:master.

README.rst Outdated
+--------------+--------------------------------+
| **type**: | ``int`` |
+--------------+--------------------------------+
| **default**: | ``False`` |
Copy link
Member

Choose a reason for hiding this comment

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

fix this line

README.rst Outdated

If a node has not been modified since the days specified and if it's link is down,
it will be deleted by the``update_topology`` management command.
Replace False with an integer to enable the feature.
Copy link
Member

Choose a reason for hiding this comment

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

Use

``False``

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 6cc5da3 on R9295:del-old-nodes into 235ad08 on netjson:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Hey @R9295,

only now that I wanted to merge this I realized this can't really work in the real world.

In short, we can implement this in a much simpler way:
we can query for nodes who have an old date (like you are doing now) but which don't have any link (right now we check if they have both link with status 'down'); we can consider these nodes as expired because they had links which were down and have been deleted by Link.delete_expired_links().

So a node is expired when not only has very old modified date (higher than the expiration days set), but also when it has no links to it.

The query to obtain such nodes would be:

Node.objects.filter(modified__lt=expiration_date,
                    source_link_set__isnull=True,
                    target_link_set__isnull=True)

Once you get these nodes you can simply delete them if the feature is enabled.

This reasoning implies that this feature depends on NETJSONGRAPH_LINK_EXPIRATION being active.

Could you implement this changes?
I'd really like to add this feature. Let me know.

Federico

@R9295
Copy link
Contributor Author

R9295 commented Feb 21, 2018

@nemesisdesign Yep, I'll implement it

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Excellent, the implementation also looks a lot cleaner.

I will test it on a real network and let you know if it works well :-)

@nemesifier nemesifier merged commit ebd8b39 into openwisp:master Feb 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature] Automatic removal of old nodes

3 participants