Allow configuring timeout for external sources #1812
Conversation
|
@pouyan021 Approved and running checks now - Thank you so much for the contribution! |
|
@pouyan021 looks like there is a small change needed where |
5cc6f4b to
e5ed88d
Compare
|
@spiffcs Appreciate your support, thanks a lot! The problem with go.mod is addressed but I forgot to sign-off my commit! I fixed that and pushed again. Could you kindly approve the checks once more? |
There was a problem hiding this comment.
Everything else LGTM - Just waiting for @wagoodman and his final say on the config direction he wants to go here
| base-url: https://search.maven.org/solrsearch/select | ||
| abort-after: 5m #override the global config |
There was a problem hiding this comment.
cc @wagoodman - I know he's pretty sensitive to duplicate fields that override each other so I'd like him to chime in on where he sees this going or what his preference would be
There was a problem hiding this comment.
Overall the functionality looks good, but have some comments on testing and configuration. I'll push up some changes shortly to help out.
I do think that RequestTimeout is a better name for this config item -- what do you think @pouyan021 ?
| @@ -278,9 +278,11 @@ feature is currently disabled by default. To enable this feature add the followi | |||
| ```yaml | |||
| external-sources: | |||
| enable: true | |||
| abort-after: 10m | |||
There was a problem hiding this comment.
I think we can clarify what this means by changing the name some. This could be interpreted as either:
a. aborting looking up from external sources in general after the duration elapses
b. aborting a single request to an external source after the duration elapses
From the functionality implemented b is implied.
Regarding naming and the above context, request-timeout feels like a more descriptive name.
ca5d50c to
0bf64dd
Compare
|
note: I force pushed to get this branch rebased onto the latest commit on main |
Hey @wagoodman thanks a lot for the thorough review and your additional changes. The name was chosen as |
|
Hey @wagoodman should I go for the |
|
Hey @wagoodman @spiffcs I resolved the conflict on this branch recently, any plans for this to move forward? |
|
@pouyan021 I'm so sorry here - I've approved and have run the final checks and will get this merged - I did not see the notification for this and apologize for letting it sit I'll get the commits in the wrap this up 😄 |
|
Hey @spiffcs, I rebased this branch again, could you please trigger the checks so we can hopefully merge this? |
|
cc @wagoodman |
|
Hi @pouyan021, it looks like this PR has been through a lot! There have been some other enhancements to the Maven search function, including rate limiting, but I think that's not exactly the same thing as you are hoping to get merged here. Another change will currently result in a scan failure if a user does end up getting rate limited. I'd like to make sure this doesn't get lost in the noise, and get it moved forward if we can. I hate to ask you to rebase this again; let me know if you'd like help getting this to the finish line. It seems like we might need to think about how all these options/behaviors should work together. |
Signed-off-by: Pouyan Khodabakhsh <pouyan021@gmail.com>
Signed-off-by: Pouyan Khodabakhsh <pouyan021@gmail.com>
… functionality Signed-off-by: Pouyan Khodabakhsh <pouyan021@gmail.com>
…-after functionality Signed-off-by: Pouyan Khodabakhsh <pouyan021@gmail.com>
Signed-off-by: Pouyan Khodabakhsh <pouyan021@gmail.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Keith Zantow <kzantow@gmail.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
…andling Signed-off-by: Pouyan Khodabakhsh <pouyan021@gmail.com>
75a337c to
4194a93
Compare
|
@kzantow It's been quite a while since I worked on this and as you mentioned many things have changed. I rebased though the best way I could. Hope it helps |
This pull request closes #1624. It adds and enforces the ability to set a new property
abort-aftertoexternalsources. As discussed in the issue, it supports both a global prop and amavenproperty that overrides the global if it is set.