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.

Sign up for GitHub

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

Jump to bottom

Migrate resource monitors (fixed heap & injected resource ) , redis and thrift_proxy filter to api v3 #13814

Closed
wants to merge 38 commits into from
Closed

Migrate resource monitors (fixed heap & injected resource ) , redis and thrift_proxy filter to api v3 #13814

wants to merge 38 commits into from

Conversation

ankatare
Copy link
Contributor

@ankatare ankatare commented Oct 29, 2020 •
edited
Loading

Commit Message: Migrate resource monitors (fixed heap & injected resource ) , redis and thrift_proxy filter to api v3
Additional Description: Copying remaining v2 packages under proto_library ove to v3 trees. which also fulfil requirements of #12841.
Risk Level: LOW
Testing: proto_format test
Docs Changes: Yes
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Optional Fixes #12841
[Optional Deprecated:]

@ankatare
v2 packages to v3
8a577f9 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
Fixing bug 12841
6f8f289 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13814 was opened by ankatare.

see: more, trace.

@yanavlasov
Copy link
Contributor

I think you have migrated quite a bit more than just fixed heap resource monitor. Can you update the title and description to match the scope of your change, please?
You will need to fix format errors in the new files. Please run tools/code_format/check_format.py check to see the errors.

@yanavlasov
Copy link
Contributor

/wait

@ankatare
Copy link
Contributor Author

@yanavlasov thanks for comments. i updated Additional Description part. please let me know if it is ok now !!

@ankatare
Copy link
Contributor Author

@yanavlasov moreover yes, i will run script as you suggested to fix build issues.

@ankatare
Copy link
Contributor Author

sorry !!! seems , some local env issues on build . looking into it !!!

@ankatare ankatare force-pushed the Migrate_FixedHeap_resource_monitor_toapi_v3 branch from 6ffa202 to 7dfa9ad Compare October 31, 2020 01:09
@yanavlasov
Copy link
Contributor

Would you be able to update documentation to reference newly versioned proto files as well please? For instance thrift router doc still refers to the v2alpha1 proto. See here:

@yanavlasov
Copy link
Contributor

/wait

@ankatare
Copy link
Contributor Author

ankatare commented Nov 3, 2020

@yanavlasov OK.

@ankatare
Copy link
Contributor Author

ankatare commented Nov 3, 2020

currently looking at build issues https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/56106/logs/24

@ankatare
fixing format issues in build
57df902 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare ankatare force-pushed the Migrate_FixedHeap_resource_monitor_toapi_v3 branch from 7dfa9ad to 57df902 Compare November 3, 2020 19:02
@htuch
Copy link
Member

htuch commented Nov 9, 2020

@ankatare please ping me back when you have format and docs passing. Thanks.

@ankatare
Copy link
Contributor Author

@htuch sure.

@ankatare
fixing proto issue in build
2a5b3e0 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
Copy link
Contributor Author

seems docs issue again :( checking it locally

@ankatare
local changes for proto format fix
7b21edc 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
proto format locally
33bcc2c 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
proto format fix for redis
a714e4b 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
local changes
c0337cc 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
Copy link
Contributor Author

still working on fixing issues in my local env. will try to close it ASAP

htuch
htuch reviewed Jan 6, 2021
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks good, but still needs unit tests.

@ankatare
incorporated review commnets of putting logic inside api_type_oracle.cc
af59960 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
Copy link
Contributor Author

ankatare commented Jan 8, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13814 (comment) was created by @ankatare.

see: more, trace.

@ankatare
Copy link
Contributor Author

ankatare commented Jan 8, 2021

really sorry for this build failure. rechecking in my local env

@ankatare
incorporated review commnets of putting logic inside api_type_oracle.cc
8cec235 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare ankatare force-pushed the Migrate_FixedHeap_resource_monitor_toapi_v3 branch from af59960 to 8cec235 Compare January 8, 2021 10:41
htuch
htuch suggested changes Jan 11, 2021
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks good, just needs some tests.
/wait

@ankatare
Copy link
Contributor Author

@htuch unit test for api_type_oracle.cc is done. please review

@ankatare
incorporated review comments and wrote unit test for api_type_oracle,…
9d055be 
… also tracking it with issue as envoyproxy#14197

Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare ankatare force-pushed the Migrate_FixedHeap_resource_monitor_toapi_v3 branch from 32c7bb3 to 9d055be Compare January 11, 2021 10:37
htuch
htuch previously approved these changes Jan 13, 2021
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ankatare
incorporated review comments and wrote unit test for api_type_oracle,…
7ab40e5 
… also tracking it with issue as envoyproxy#14197

Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
Merge branch 'Migrate_FixedHeap_resource_monitor_toapi_v3' of https:/…
89cac3c 
…/github.com/ankatare/envoy into Migrate_FixedHeap_resource_monitor_toapi_v3
@ankatare
Copy link
Contributor Author

pushed code again (with latest merge master) for fixing errors in windows

Base automatically changed from master to main January 15, 2021 23:01
@ankatare ankatare mentioned this pull request Jan 17, 2021
Closed
@ankatare
Copy link
Contributor Author

cc @envoyproxy/windows-dev for windows issue.

@ankatare
putting debug log to trace the issue
b448eb1 
Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
Copy link
Contributor Author

@htuch Hi, i don't know what is meaning of "ankatare dismissed htuch’s stale review via b448eb1 ". i just pushed some debug log as suggested by community. ( @davinci26 )

@htuch
Copy link
Member

htuch commented Jan 20, 2021

I've reassigned the issue my way, will close out this PR. Thanks for the help and I'll use this as the basis.

@htuch htuch closed this Jan 20, 2021
@ankatare ankatare deleted the Migrate_FixedHeap_resource_monitor_toapi_v3 branch February 12, 2021 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dio dio dio left review comments

@htuch htuch htuch left review comments

Assignees

@yanavlasov yanavlasov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate FixedHeap resource monitor to api v3
5 participants
@ankatare @yanavlasov @htuch @rgs1 @dio