search

apache / shenyu

Apache ShenYu is a Java native API Gateway for service proxy, protocol conversion and API governance.
https://shenyu.apache.org/
Apache License 2.0
8.43k stars 2.93k forks source link

[Question] For Resilience4J plugin, why using max value between user configuration and preset, instead of making the preset value as default? #2927

Closed hgaol closed 2 years ago

hgaol commented 2 years ago

Question

You can see below codes in Resilience4JHandle#checkData. The question is as mentioned in title. Is it by design?

image

BTW, if it's purpose is to use preset value as default, I can help to fix it.

Similar question, #2055 .

hgaol commented 2 years ago

Sorry, seems all the configurations except fallbackUri are required in admin portal. I guess the goal for checkData is to avoid invalid number, like -10?

yu199195 commented 2 years ago

it set value, i think the logic should be to set defaults if you don't have them and configure them if you do~

hgaol commented 2 years ago

it set value, i think the logic should be to set defaults if you don't have them and configure them if you do~

If like you mentioned, I think checkData method needs to be updated. BTW, many fields like limitForPeriod are int or long, which means it will always has a number. Considering 0 is also an valid config in resilience4j for some scenarios, could I just verify the number and replace with default when it's less than 0. Like,

resilience4JHandle.setLimitForPeriod(resilience4JHandle.getLimitForPeriod() < 0 ? Constants.LIMIT_FOR_PERIOD : resilience4JHandle.getLimitForPeriod());
yu199195 commented 2 years ago

could I just verify the number and replace with default when it's less than 0. Like

yes , you can~