[GitHub] lucene-solr pull request: LUCENE-6622: Fixed resource leak

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request: LUCENE-6622: Fixed resource leak

asfgit
GitHub user rmp91 opened a pull request:

    https://github.com/apache/lucene-solr/pull/169

    LUCENE-6622: Fixed resource leak

    https://issues.apache.org/jira/browse/LUCENE-6622

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/rmp91/lucene-solr LUCENE-6622

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/lucene-solr/pull/169.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #169
   
----
commit 0b163bf840a14a0d0fb435d40cacb9b7bfec3869
Author: Rishabh Patel <[hidden email]>
Date:   2015-06-26T22:02:51Z

    Fixed resource leak

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #169: Add status return for replication

asfgit
Github user dsmiley commented on the issue:

    https://github.com/apache/lucene-solr/pull/169
 
    @millerjeff0  when you created this PR, you didn't put SOLR-10249 in *in the title* and therefore the PR and the Jira issue aren't linked.  Can you edit it and do that please?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user millerjeff0 commented on the issue:

    https://github.com/apache/lucene-solr/pull/169
 
    Thanks I was not aware I needed to do so,  fixed it


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107514287
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -161,6 +163,52 @@
     
       private Integer soTimeout;
     
    +  private static final String INTERRUPT_RESPONSE_MESSAGE = "Interrupted while waiting for modify lock";
    +
    +  public static class IndexFetchResult {
    +    private final String message;
    +    private final boolean status;
    --- End diff --
   
    perhaps rename "status" to "successful" (getter too)?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107513516
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -321,9 +369,16 @@ boolean fetchLatestIndex(boolean forceReplication, boolean forceCoreReload) thro
           try {
             response = getLatestVersion();
           } catch (Exception e) {
    -        LOG.error("Master at: " + masterUrl + " is not available. Index fetch failed. Exception: " + e.getMessage());
    -        return false;
    -      }
    +        final String errorMsg = e.getMessage();
    --- End diff --
   
    Granted e.getMessage() was what this code was doing before you made changes but can you please use e.toString() instead?  IMO, e.getMessage() should rarely if ever be called instead of e.toString() because e.toString() critically contains the name of the exception itself.  The "message" alone can be unclear without the exception class.  Consider `FileNotFoundException` and there are others.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107512860
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -311,7 +359,7 @@ boolean fetchLatestIndex(boolean forceReplication, boolean forceCoreReload) thro
             Replica replica = getLeaderReplica();
             CloudDescriptor cd = solrCore.getCoreDescriptor().getCloudDescriptor();
             if (cd.getCoreNodeName().equals(replica.getName())) {
    -          return false;
    +          return IndexFetchResult.CORE_NODE_IS_REPLICA;
    --- End diff --
   
    Maybe call this NOT_LEADER ?  Even a leader is a replica (it's the _role_ a replica plays).  Granted many of us promote this ambiguity because we haven't clearly/consistently labelled a non-leader, e.g. a follower.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107514170
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -161,6 +163,52 @@
     
       private Integer soTimeout;
     
    +  private static final String INTERRUPT_RESPONSE_MESSAGE = "Interrupted while waiting for modify lock";
    +
    +  public static class IndexFetchResult {
    --- End diff --
   
    I really like how you modeled this return object.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user millerjeff0 commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107526774
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -321,9 +369,16 @@ boolean fetchLatestIndex(boolean forceReplication, boolean forceCoreReload) thro
           try {
             response = getLatestVersion();
           } catch (Exception e) {
    -        LOG.error("Master at: " + masterUrl + " is not available. Index fetch failed. Exception: " + e.getMessage());
    -        return false;
    -      }
    +        final String errorMsg = e.getMessage();
    --- End diff --
   
    Great point, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user tflobbe commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107954150
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -161,6 +163,52 @@
     
       private Integer soTimeout;
     
    +  private static final String INTERRUPT_RESPONSE_MESSAGE = "Interrupted while waiting for modify lock";
    +
    +  public static class IndexFetchResult {
    +    private final String message;
    +    private final boolean successful;
    +    private final Throwable exception;
    +
    +    public static final String FAILED_BY_INTERRUPT_MESSAGE = "Fetching index failed by interrupt";
    +    public static final String FAILED_BY_EXCEPTION_MESSAGE = "Fetching index failed by exception";
    +
    +    /** pre-defined results */
    +    public static final IndexFetchResult ALREADY_IN_SYNC = new IndexFetchResult("Local index commit is already in sync with peer", true, null);
    +    public static final IndexFetchResult INDEX_FETCH_FAILURE = new IndexFetchResult("Fetching lastest index is failed", false, null);
    +    public static final IndexFetchResult INDEX_FETCH_SUCCESS = new IndexFetchResult("Fetching latest index is successful", true, null);
    +    public static final IndexFetchResult LOCK_OBTAIN_FAILED = new IndexFetchResult("Obtaining SnapPuller lock failed", false, null);
    +    public static final IndexFetchResult MASTER_VERSION_ZERO = new IndexFetchResult("Index in peer is empty and never committed yet", true, null);
    +    public static final IndexFetchResult NO_INDEX_COMMIT_EXIST = new IndexFetchResult("No IndexCommit in local index", false, null);
    +    public static final IndexFetchResult PEER_INDEX_COMMIT_DELETED = new IndexFetchResult("No files to download because IndexCommit in peer was deleted", false, null);
    +    public static final IndexFetchResult LOCAL_ACTIVITY_DURING_REPLICATION = new IndexFetchResult("Local index modification during replication", false, null);
    +    public static final IndexFetchResult CORE_NODE_IS_NOT_LEADER = new IndexFetchResult("Core Name Name Equals Leader Replica Name", false, null);
    --- End diff --
   
    This message (and variable name) seems wrong. Looking at the case where this is thrown, it seems like this would happen if "OnlyLeaderIndexes" feature is being used, and this replica is the current leader. Message should be something like"Replicating from leader but I'm the shard leader"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107955318
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -161,6 +163,52 @@
     
       private Integer soTimeout;
     
    +  private static final String INTERRUPT_RESPONSE_MESSAGE = "Interrupted while waiting for modify lock";
    +
    +  public static class IndexFetchResult {
    +    private final String message;
    +    private final boolean successful;
    +    private final Throwable exception;
    +
    +    public static final String FAILED_BY_INTERRUPT_MESSAGE = "Fetching index failed by interrupt";
    +    public static final String FAILED_BY_EXCEPTION_MESSAGE = "Fetching index failed by exception";
    +
    +    /** pre-defined results */
    +    public static final IndexFetchResult ALREADY_IN_SYNC = new IndexFetchResult("Local index commit is already in sync with peer", true, null);
    +    public static final IndexFetchResult INDEX_FETCH_FAILURE = new IndexFetchResult("Fetching lastest index is failed", false, null);
    +    public static final IndexFetchResult INDEX_FETCH_SUCCESS = new IndexFetchResult("Fetching latest index is successful", true, null);
    +    public static final IndexFetchResult LOCK_OBTAIN_FAILED = new IndexFetchResult("Obtaining SnapPuller lock failed", false, null);
    +    public static final IndexFetchResult MASTER_VERSION_ZERO = new IndexFetchResult("Index in peer is empty and never committed yet", true, null);
    +    public static final IndexFetchResult NO_INDEX_COMMIT_EXIST = new IndexFetchResult("No IndexCommit in local index", false, null);
    +    public static final IndexFetchResult PEER_INDEX_COMMIT_DELETED = new IndexFetchResult("No files to download because IndexCommit in peer was deleted", false, null);
    +    public static final IndexFetchResult LOCAL_ACTIVITY_DURING_REPLICATION = new IndexFetchResult("Local index modification during replication", false, null);
    +    public static final IndexFetchResult CORE_NODE_IS_NOT_LEADER = new IndexFetchResult("Core Name Name Equals Leader Replica Name", false, null);
    --- End diff --
   
    Thanks @tflobbe .  I can make this simple change before committing today.  Perhaps the constant should be SELF_LEADER_REPLICATE or EXPECTING_NON_LEADER ?  Be my guest to make suggestions.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #169: Add status return for replication SOLR-10249

asfgit
In reply to this post by asfgit
Github user tflobbe commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/169#discussion_r107959546
 
    --- Diff: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java ---
    @@ -161,6 +163,52 @@
     
       private Integer soTimeout;
     
    +  private static final String INTERRUPT_RESPONSE_MESSAGE = "Interrupted while waiting for modify lock";
    +
    +  public static class IndexFetchResult {
    +    private final String message;
    +    private final boolean successful;
    +    private final Throwable exception;
    +
    +    public static final String FAILED_BY_INTERRUPT_MESSAGE = "Fetching index failed by interrupt";
    +    public static final String FAILED_BY_EXCEPTION_MESSAGE = "Fetching index failed by exception";
    +
    +    /** pre-defined results */
    +    public static final IndexFetchResult ALREADY_IN_SYNC = new IndexFetchResult("Local index commit is already in sync with peer", true, null);
    +    public static final IndexFetchResult INDEX_FETCH_FAILURE = new IndexFetchResult("Fetching lastest index is failed", false, null);
    +    public static final IndexFetchResult INDEX_FETCH_SUCCESS = new IndexFetchResult("Fetching latest index is successful", true, null);
    +    public static final IndexFetchResult LOCK_OBTAIN_FAILED = new IndexFetchResult("Obtaining SnapPuller lock failed", false, null);
    +    public static final IndexFetchResult MASTER_VERSION_ZERO = new IndexFetchResult("Index in peer is empty and never committed yet", true, null);
    +    public static final IndexFetchResult NO_INDEX_COMMIT_EXIST = new IndexFetchResult("No IndexCommit in local index", false, null);
    +    public static final IndexFetchResult PEER_INDEX_COMMIT_DELETED = new IndexFetchResult("No files to download because IndexCommit in peer was deleted", false, null);
    +    public static final IndexFetchResult LOCAL_ACTIVITY_DURING_REPLICATION = new IndexFetchResult("Local index modification during replication", false, null);
    +    public static final IndexFetchResult CORE_NODE_IS_NOT_LEADER = new IndexFetchResult("Core Name Name Equals Leader Replica Name", false, null);
    --- End diff --
   
    Yes, those sound good. I personally like **EXPECTING_NON_LEADER** better


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]