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

Clear ApplicationLogEvent and SkaffoldLogEvent buffers on new dev iteration #5993

Merged
merged 2 commits into from
Jun 9, 2021

Conversation

MarlonGamez
Copy link
Contributor

Related: #5368 #5370

Description
This change resets the buffers containing SkaffoldLogEvents and ApplicationLogEvents on a new dev iteration, to prevent increased RAM usage over time.

@MarlonGamez MarlonGamez requested a review from a team as a code owner June 9, 2021 19:05
@MarlonGamez MarlonGamez requested a review from briandealwis June 9, 2021 19:05
@google-cla google-cla bot added the cla: yes label Jun 9, 2021
@MarlonGamez MarlonGamez changed the title Clear user application and skaffold log events on new dev iteration Clear ApplicationLogEvent and SkaffoldLogEvent buffers on new dev iteration Jun 9, 2021
@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #5993 (8f14bd0) into master (4afe45b) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5993      +/-   ##
==========================================
- Coverage   70.76%   70.66%   -0.10%     
==========================================
  Files         460      462       +2     
  Lines       17746    17804      +58     
==========================================
+ Hits        12558    12582      +24     
- Misses       4260     4289      +29     
- Partials      928      933       +5     
Impacted Files Coverage Δ
pkg/skaffold/event/v2/event.go 76.38% <100.00%> (+0.22%) ⬆️
pkg/skaffold/deploy/deploy_mux.go 75.67% <0.00%> (-9.18%) ⬇️
pkg/skaffold/util/tar.go 50.66% <0.00%> (-5.34%) ⬇️
pkg/skaffold/kubernetes/logger/log.go 37.50% <0.00%> (-4.71%) ⬇️
pkg/skaffold/kubernetes/watcher.go 80.48% <0.00%> (-3.89%) ⬇️
pkg/skaffold/docker/image.go 78.13% <0.00%> (-1.40%) ⬇️
pkg/skaffold/runner/v1/deploy.go 53.00% <0.00%> (-0.47%) ⬇️
pkg/skaffold/runner/v1/dev.go 72.98% <0.00%> (-0.13%) ⬇️
pkg/skaffold/deploy/kpt/kpt.go 81.75% <0.00%> (-0.10%) ⬇️
pkg/skaffold/deploy/helm/deploy.go 79.16% <0.00%> (-0.08%) ⬇️
... and 8 more

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 4afe45b...8f14bd0. Read the comment docs.

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

should we allocate new memory instead of nil to make sure there is no nil point exception

@MarlonGamez
Copy link
Contributor Author

@tejal29 I'm unsure, since there are no fields of a slice that we can access so I don't think that could happen. I think that functionally, a nil and empty slice would behave the same. Iterating over it just results in zero iterations, like an empty slice.
This way is just cleaner to me over writing out a slice literal. I'm happy to change if you prefer tho!

@tejal29
Copy link
Contributor

tejal29 commented Jun 9, 2021

@tejal29 I'm unsure, since there are no fields of a slice that we can access so I don't think that could happen. I think that functionally, a nil and empty slice would behave the same. Iterating over it just results in zero iterations, like an empty slice.
This way is just cleaner to me over writing out a slice literal. I'm happy to change if you prefer tho!

ah. I guess iterating would be fine. I had looked up usage last time and looks like we pass the pointer to these fields.

return ev.forEach(&ev.applicationLogListeners, &ev.applicationLogs, &ev.applicationLogsLock, callback)

My concern is this might end up into nil pointer exception?

@MarlonGamez
Copy link
Contributor Author

@tejal29 made the change to use slice literals 👍🏼

@tejal29 tejal29 merged commit 840f9b2 into GoogleContainerTools:master Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants