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

Retry policy support for v2 #182

Merged
merged 16 commits into from
Jul 5, 2023
Merged

Retry policy support for v2 #182

merged 16 commits into from
Jul 5, 2023

Conversation

gavin-aguiar
Copy link
Contributor

@gavin-aguiar gavin-aguiar commented Jun 8, 2023

Added support for retry policy for V2 programming model.

PR includes adding a new SettingApi which is similar to TriggerApi and BindingsApi.
Function Name and Retry decorators will be part of the SettingsApi

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #182 (72d353f) into dev (5ef605e) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #182      +/-   ##
==========================================
+ Coverage   90.50%   90.70%   +0.20%     
==========================================
  Files         106       55      -51     
  Lines        6256     3185    -3071     
  Branches     1097      822     -275     
==========================================
- Hits         5662     2889    -2773     
+ Misses        439      219     -220     
+ Partials      155       77      -78     
Flag Coverage Δ
unittests 90.67% <100.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
azure/functions/decorators/__init__.py 100.00% <ø> (ø)
azure/functions/__init__.py 100.00% <100.00%> (ø)
azure/functions/decorators/core.py 98.86% <100.00%> (+0.23%) ⬆️
azure/functions/decorators/function_app.py 99.30% <100.00%> (+0.03%) ⬆️
azure/functions/decorators/function_name.py 100.00% <100.00%> (ø)
azure/functions/decorators/retry_policy.py 100.00% <100.00%> (ø)

... and 53 files with indirect coverage changes

@gavin-aguiar gavin-aguiar force-pushed the gaaguiar/retry_policy branch from d087564 to 9880a04 Compare June 16, 2023 19:18
@gavin-aguiar gavin-aguiar marked this pull request as ready for review June 20, 2023 19:00
return setting
return None

def get_settings_json(self, setting_name) -> Optional[Dict]:
Copy link
Member

Choose a reason for hiding this comment

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

Change this to get_setting_dict rather than json as this isn't serializing the json.

@@ -145,12 +176,12 @@ def get_user_function(self) -> Callable[..., Any]:
"""
return self._func

def get_function_name(self) -> str:
"""Get the function name.
# def get_function_name(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

remove if not used

def function_name(self, name: str,
setting_extra_fields: Dict[str, Any] = {},
) -> Callable[..., Any]:
"""Set name of the :class:`Function` object.
Copy link
Member

Choose a reason for hiding this comment

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

Add that this is optional and if not set, we will pickup the method name

**setting_extra_fields))
return fb

return decorator()
Copy link
Member

Choose a reason for hiding this comment

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

Are we planning to add validation to ensure basic combinations are not missed? Better to raise it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Validations will be in the host. If we add a validation here it will hide the host error and only show no functions found in the logs

Copy link
Member

Choose a reason for hiding this comment

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

We should at least send a warning - better wording of the validation

Copy link
Member

Choose a reason for hiding this comment

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

Discussed this offline can validations across the library coming later.

@@ -1987,7 +2086,7 @@ def register_functions(self, function_container: DecoratorApi) -> None:
register_blueprint = register_functions


class FunctionApp(FunctionRegister, TriggerApi, BindingApi):
class FunctionApp(FunctionRegister, TriggerApi, BindingApi, SettingsApi):
Copy link
Member

Choose a reason for hiding this comment

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

This is becoming more complicated day by day

Copy link
Member

Choose a reason for hiding this comment

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

Curious if we need to maintain this file - or a way to autogenerate this - using sphinx thing that I was trying.

@@ -116,6 +117,36 @@ def get_bindings(self) -> List[Binding]:
"""
return self._bindings

def get_setting(self, setting_name: str) -> Optional[Setting]:
Copy link
Member

Choose a reason for hiding this comment

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

Where do we get the list of all the settings registered to the Function? where you can get the setting_name itself from? I don't see it within a Function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class Functions has a List[Settings] which should have all the settings for a function

All optional fields will be given default value by function host when
they are parsed by function host.

Ref: https://aka.ms/azure-function-retry <FIX LINK>
Copy link
Member

Choose a reason for hiding this comment

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

FIX LINK before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

**setting_extra_fields))
return fb

return decorator()
Copy link
Member

Choose a reason for hiding this comment

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

We should at least send a warning - better wording of the validation

class TestSettings(unittest.TestCase):

def test_setting_creation(self):
test_setting = DummySetting(setting_type="TestSetting")
Copy link
Member

Choose a reason for hiding this comment

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

This seems a little counter intuitive -
DummySetting(setting_type="TestSetting") - it is implicitly picking up the name of setting ("setting_type")

Can you show how the function_name setting will be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and added comments

**setting_extra_fields))
return fb

return decorator()
Copy link
Member

Choose a reason for hiding this comment

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

Discussed this offline can validations across the library coming later.

@gavin-aguiar gavin-aguiar merged commit 3235036 into dev Jul 5, 2023
@gavin-aguiar gavin-aguiar deleted the gaaguiar/retry_policy branch July 5, 2023 20:00
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