[
https://issues.apache.org/jira/browse/CONNECTORS-1730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17601664#comment-17601664
]
Karl Wright commented on CONNECTORS-1730:
-----------------------------------------
Hi, when an enhancement request is made it's important to understand why the
enhancement is needed. Are there situations you have encountered where the
numbers that are hardcoded are a problem? Or do you just not like hardcoded
numbers?
We have many cases of hardwired retry behavior in all of our connectors. See
all places where ServiceInterruption exception is thrown, for example. These
are generally modified only by developers because it would be far too complex
to support these rules in either the UI or in configuration information. The
complaints are not about the retry parameters, but usually around how we handle
very specific kinds of errors. There are many requests to "improve" connectors
by making them completely ignore errors, for example - and these are generally
not well thought out.
The burden is on you to demonstrate why these numbers need to be configurable.
> Improvement suggestion for retry function in SharedDriveConnector
> -----------------------------------------------------------------
>
> Key: CONNECTORS-1730
> URL: https://issues.apache.org/jira/browse/CONNECTORS-1730
> Project: ManifoldCF
> Issue Type: Improvement
> Reporter: Nguyen Huu Nhat
> Assignee: Karl Wright
> Priority: Major
>
> Hi there,
> I would like to suggest the following retry-related improvements:
> h3. +*1. Connector name*+
> SharedDriveConnector
> h3. +*2. Overview*+
> * When connection to SMB can't be executed, JCIFS connector will fail to
> connect and exception will occur. JCIFS will attempt to retry, and abort
> after a certain number of time.
> * The number of retry is currently controlled by the following parameters:
> ** *retriesRemaining* (hardcode：3)： The number of occurence of the same
> exception for a file or method. If a different exception occurs, this value
> is reset to 3.
> ** *totalTries* (hardcode：5)： The total number of occurrence of an exception
> for a file or method.
> For the two variables above, if *retriesRemaining* becomes 0 or *totalTries*
> becomes 5 then the job will be aborted.
> h3. +*3. Reasons for improvement*+
> Currently the maximum number of retry is being hardcoded at 3 and 5,
> respectively.
> In case connection to file server is unstable, to avoid aborting, I would
> like to suggest making these values customizable.
> For implementation, I would like to suggest the following methods:
> * 1/ Setting retry values in *properties.xml*
> * 2/ Setting retry values on WebUI of repository connection
> Between the two methods above, I suggest the first method because of
> following reasons:
> * The first method is easier to implement
> * Although the second method is more user-friendly, there are several issues:
> ** The config data from the screen will have to be stored in the database
> (PostgreSQL), resulting in an increased number of fields.
> ** Consequently, there might be a need to perform DB Migration in case
> further changes to setting field are needed.
> ※ According to the above reasons, I will proceed with the first method
> 'Setting retry values in properties.xml' for the next part of this suggestion.
> h3. +*4. Improvement*+
> Changing source code to read maximum number of retries from *properties.xml*
> Declare two variables in *properties.xml* to set the maximum number of retry:
> *
> `org.apache.manifoldcf.crawler.connectors.sharedrive.consecutivesmbexceptionretrycount`
> ⇒ Set to `consecutiveSMBExceptionRetryCount`
> * `org.apache.manifoldcf.crawler.connectors.sharedrive.totalsmbretrycount`
> ⇒ Set to `totalSMBRetryCount`
> E.g:
> {code:xml}
> <property
> name="org.apache.manifoldcf.crawler.connectors.sharedrive.consecutivesmbexceptionretrycount"
> value="3"/>
> <property
> name="org.apache.manifoldcf.crawler.connectors.sharedrive.totalsmbretrycount"
> value="5"/>
> {code}
> SharedDriveConnector will load these values from the file and set to two
> variables within the source code.
> ※In case these values can't be found from the file or set to an invalid
> value, default values will be used instead.
> h3. +*5. Suggested source code (based on release 2.22.1)*+
> Target class:
> org.apache.manifoldcf.crawler.connectors.sharedrive.SharedDriveConnector
> * Declare two class variables to store the configured values as follows:
> [https://github.com/apache/manifoldcf/blob/release-2.22.1/connectors/jcifs/connector/src/main/java/org/apache/manifoldcf/crawler/connectors/sharedrive/SharedDriveConnector.java#L103]
>
> {code:java}
> private final static int consecutiveSMBExceptionRetryCount;
> private final static int totalSMBRetryCount;
> {code}
>
> * Initialize the two variables above with following steps:
> ** Set the values configured in 'properties.xml' to the two variables above
> ** If these values weren't configured or invalid, set them to default values
> of 3 and 5, respectively.
> [https://github.com/apache/manifoldcf/blob/release-2.22.1/connectors/jcifs/connector/src/main/java/org/apache/manifoldcf/crawler/connectors/sharedrive/SharedDriveConnector.java#L106]
> {code:java}
> // Static initialization of various system properties. This hopefully
> takes place
> // before jcifs is loaded.
> static
> {
> ...
> int tempConsecutiveSMBExceptionRetryCount = 3;
> int tempTotalSMBRetryCount = 5;
> try {
> tempConsecutiveSMBExceptionRetryCount =
> ManifoldCF.getIntProperty("org.apache.manifoldcf.crawler.connectors.sharedrive.consecutivesmbexceptionretrycount",
> tempConsecutiveSMBExceptionRetryCount);
> } catch (ManifoldCFException e) {
> Logging.connectors.warn("Invalid property value for " +
> "org.apache.manifoldcf.crawler.connectors.sharedrive.consecutivesmbexceptionretrycount,
> must be integer. Setting to default: " +
> Integer.toString(tempConsecutiveSMBExceptionRetryCount));
> }
> consecutiveSMBExceptionRetryCount =
> tempConsecutiveSMBExceptionRetryCount;
> try {
> tempTotalSMBRetryCount =
> ManifoldCF.getIntProperty("org.apache.manifoldcf.crawler.connectors.sharedrive.totalsmbretrycount",
> tempTotalSMBRetryCount);
> } catch (ManifoldCFException e) {
> Logging.connectors.warn("Invalid property value for " +
> "org.apache.manifoldcf.crawler.connectors.sharedrive.totalsmbretrycount, must
> be integer. Setting to default: " + Integer.toString(tempTotalSMBRetryCount));
> }
> totalSMBRetryCount = tempTotalSMBRetryCount;
> }
> {code}
> * In the following methods, implementation to use
> [consecutiveSMBExceptionRetryCount], [totalSMBRetryCount] initialized above
> instead of hardcoded values is needed:
>
> ** fileExists(SmbFile file)
> ** fileIsDirectory(SmbFile file)
> ** fileLastModified(SmbFile file)
> ** fileLength(SmbFile file)
> ** fileListFiles(SmbFile file, SmbFileFilter filter)
> ** getFileInputStream(SmbFile file)
> ** getFileSecurity(SmbFile file, boolean useSIDs)
> ** getFileShareSecurity(SmbFile file, boolean useSIDs)
> ** getFileType(SmbFile file)
> * Change to *fileExists()* example
> [https://github.com/apache/manifoldcf/blob/release-2.22.1/connectors/jcifs/connector/src/main/java/org/apache/manifoldcf/crawler/connectors/sharedrive/SharedDriveConnector.java#L2176]
> {code:java}
> protected static boolean fileExists(SmbFile file)
> throws SmbException
> {
> int totalTries = 0;
> - int retriesRemaining = 3;
> + int retriesRemaining = consecutiveSMBExceptionRetryCount;
> SmbException currentException = null;
> - while (retriesRemaining > 0 && totalTries < 5)
> + while (retriesRemaining > 0 && totalTries < totalSMBRetryCount)
> {
> retriesRemaining--;
> totalTries++;
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
