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

Calculating and updtating alert_count in every node sync #754

Merged
merged 2 commits into from
Mar 29, 2018

Conversation

GowthamShanmugam
Copy link
Contributor

tendrl-bug-id: #753

Signed-off-by: GowthamShanmugasundaram [email protected]

tendrl-bug-id: Tendrl#753

Signed-off-by: GowthamShanmugasundaram <[email protected]>
@GowthamShanmugam GowthamShanmugam requested a review from a team as a code owner March 28, 2018 16:51
@GowthamShanmugam
Copy link
Contributor Author

@shtripat @shirshendu @r0h4n please review

@GowthamShanmugam
Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam changed the title Calculating and updtating alert_count in each node sync Calculating and updtating alert_count in every node sync Mar 28, 2018
@GowthamShanmugam
Copy link
Contributor Author

Each node will update read its own alerts only and it will update its own alerts count only

@@ -164,10 +153,39 @@ def run(self):
sync_cluster_contexts_thread.daemon = True
sync_cluster_contexts_thread.start()
sync_cluster_contexts_thread.join()

# Update node alert count
update_node_alert_count()
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want to do this every sync as an overwrite, or re-calculate only if node is deleted due to TTL? @r0h4n comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be done only if NS.tendrl.objects.NodeAlertCounters.exists() is False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will change this

@@ -164,10 +153,39 @@ def run(self):
sync_cluster_contexts_thread.daemon = True
sync_cluster_contexts_thread.start()
sync_cluster_contexts_thread.join()

# Update node alert count
update_node_alert_count()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be done only if NS.tendrl.objects.NodeAlertCounters.exists() is False

@codecov
Copy link

codecov bot commented Mar 29, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@9a2f43a). Click here to learn what that means.
The diff coverage is 7.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #754   +/-   ##
=========================================
  Coverage          ?   42.65%           
=========================================
  Files             ?       72           
  Lines             ?     2846           
  Branches          ?      407           
=========================================
  Hits              ?     1214           
  Misses            ?     1583           
  Partials          ?       49
Impacted Files Coverage Δ
tendrl/node_agent/node_sync/__init__.py 17.54% <7.14%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a2f43a...0be73f3. Read the comment docs.

@GowthamShanmugam
Copy link
Contributor Author

@shtripat @r0h4n changes done

Copy link
Member

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

Looks fine

@@ -164,10 +153,40 @@ def run(self):
sync_cluster_contexts_thread.daemon = True
sync_cluster_contexts_thread.start()
sync_cluster_contexts_thread.join()

# Update node alert count
if not NS.tendrl.objects.NodeAlertCounters().exists():
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be increment the alert count if NS.tendrl.objects.NodeAlertCounters().exists():

right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@r0h4n When node details are deleted by ttl or initially if alert count object is not present so at that time only it will save alert count object in etcd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What i feel is let it calculate alert count in every sync because it calculating alert count only for that node only, it is not reading all alerts. So whatever happens alert count won't goes wrong

@shtripat @r0h4n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a suggestion

@r0h4n r0h4n merged commit 4541705 into Tendrl:master Mar 29, 2018
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.

3 participants