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

Refactor ForwardingJob class and fix few issues #38

Merged
merged 5 commits into from
Apr 12, 2020

Conversation

shafreenAnfar
Copy link
Contributor

Hi All,

This PR does the $subject and in the process I have fixed two issues as well.

Programmers spend more time to reading code and less time on writing code. Therefore, it is needless to say that having readable code is essential in any project. ForwardingJob class was actually the opposite of this (forgive me for saying this) and it is very hard to read the code do new modification. Making the code vulnerable for regression issues.

Therefore, I have refactored the code starting the basic principle of functions doing one and only one thing. In the processes I have moved some variables into class variables as actually control the state of the ForwadingJob. Also, code abstraction done in a way that, reader starts with the highest abstraction of the concept and then drill down to more granular abstractions. This is similar to news-paper metaphor. Also, all the functions follow vertical affinity and therefore closely related functions are placed next to other.

Please note that after the refactor I have re-tested all the scenarios such as,

Out Only Scenario

  • Success scenario
  • Retry scenario
  • Max delivery scenario
  • Max delivery drop scenario
  • Invoking Deactivate scenario

Response Scenario

  • Success scenario
  • Retry scenario
  • Retry on application level failure scenario
  • Invoking reply sequence scenario
  • Invoking fault sequence scenario
  • Invoking deactivate sequence scenario
  • Max delivery attempt scenario
  • Max delivery drop scenario

Along with this I have fixed below two bugs,

https://issues.apache.org/jira/browse/SYNAPSE-1120
https://issues.apache.org/jira/browse/SYNAPSE-1121

@isudana
Copy link
Contributor

isudana commented Apr 6, 2020

Thanks Shafreen, I will take time and review the PR.

@isudana isudana self-requested a review April 6, 2020 02:26
@isudana isudana merged commit 9a11f35 into apache:master Apr 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants