Added command timeout #26
Conversation
…y pick from development branch)
|
Any chance you can squash this into less commits? |
There was a problem hiding this comment.
(not someone who can merge in, but someone who has worked with the ones who can merge in)
love these changes. My only wishes are:
- squash commits
- have a NetconfSession overloaded builder with same signature as old one that calls the new signature and passes in the timeout to both new timeout params. If you want, you can add
https://docs.oracle.com/javase/7/docs/technotes/guides/javadoc/deprecation/deprecation.html
annotation to the old signature.
| private int connectionTimeout; | ||
| private int commandTimeout; |
|
|
||
| private static final String CANDIDATE_CONFIG = "candidate"; | ||
| private static final String EMPTY_CONFIGURATION_TAG = "<configuration></configuration>"; | ||
| private static final String RUNNING_CONFIG = "running"; | ||
| private static final String NETCONF_SYNTAX_ERROR_MSG_FROM_DEVICE = "netconf error: syntax error"; | ||
|
|
||
| NetconfSession(Channel netconfChannel, int timeout, String hello, | ||
| NetconfSession(Channel netconfChannel, int connectionTimeout, int commandTimeout, | ||
| String hello, | ||
| DocumentBuilder builder) throws IOException { |
There was a problem hiding this comment.
do you have an overloaded method to make one of these with the old signature? The github diff is not making it obvious.
It would be nice to have both, plus it would be nice to have a builder to make one of these, but that is for a future feature pr. :)
| if (st.startsWith(LF)) | ||
| st = st.substring(st.indexOf(LF) + 1); | ||
| if (st.endsWith(LF)) { | ||
| st = st.substring(0, st.lastIndexOf(LF)); |
|
|
@peterjhill |
|
I can't merge it. :) maybe the juniper devs can. As a fellow user of this library, thanks for your contributions. |
|
I think the only change I would want is to bump the version in the pom.xml from to perhaps 2.1.0 ? |
…nit test, added device emulation to unit test
Vladislav-Kisliy
force-pushed
the
pre-merge
branch
from
bc14db8 to
df348c3
Compare
|
@peterjhill Hi. Please have a look. I returned old constructor, modified builder and squashed several commits. For the rest of them I'm not sure it's even possible without recreation my fork from the scratch |
| assertThat(device.getTimeout()).isEqualTo(DEFAULT_TIMEOUT); | ||
| assertThat(device.getConnectionTimeout()).isEqualTo(DEFAULT_TIMEOUT); | ||
| assertThat(device.getCommandTimeout()).isEqualTo(DEFAULT_TIMEOUT); |
There was a problem hiding this comment.
If you do create the second constructor, then it would be great to have tests for each constructor. Sort of duplicate this test with each version.
|
I pinged someone I know at Juniper about the PR. Hopefully they can take a look at it. THanks again Vlad! |
| @@ -88,6 +89,8 @@ public Device( | |||
| @NonNull String hostName, | |||
| Integer port, | |||
| Integer timeout, | |||
There was a problem hiding this comment.
Is this still required? Where is this set?
There was a problem hiding this comment.
I think it is down here in NetconfSession:
NetconfSession(Channel netconfChannel, int connectionTimeout, int commandTimeout,
If only timeout is set, then Vlad's change sets both timeout flavors to the value of timeout.
There was a problem hiding this comment.
yes, it's for compatibility with old API
…ded array copying in BufferReader
improved a consuming performance, fixed an unit test, added device emulation to unit test