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

Fix NPE for /_cat/indices when no primary shard #26953

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Oct 10, 2017

When a node which contains the primary shard is unavailable, the primary
stats (and the total stats) of an IndexStats will be empty for a short
moment (while the primary shard is being relocated). However, we assume
that these stats are always non-empty when handling _cat/indices in
RestIndicesAction. This commit checks the content of these stats before
accessing.

Closes #26942

When a node which contains the primary shard is unavailable, the primary
stats (and the total stats) of an `IndexStats` will be empty for a short
moment (while the primary shard is being relocated). However, we assume
that these stats are always non-empty when handling `_cat/indices` in
RestIndicesAction. This commit checks the content of these stats before
accessing.

Closes elastic#26942
@dnhatn
Copy link
Member Author

dnhatn commented Oct 10, 2017

@jasontedor, I do not like having many null-checks. If there is a better approach, please let me know. Thank you.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please add version labels for v5.6.4, v6.0.0, v6.1.0 and backport there too.

@dnhatn dnhatn merged commit b63f718 into elastic:master Oct 10, 2017
@dnhatn
Copy link
Member Author

dnhatn commented Oct 10, 2017

Thanks @jasontedor. I will backport to v5.6.4, v6.0.0, and v6.1.0.

@jasontedor
Copy link
Member

@dnhatn It's fine. Maybe we could do:

    private <T, U> U ifNotNull(final CommonStats stats, Function<CommonStats, T> f, Function<T, U> g) {
        final T x = f.apply(stats);
        return x == null ? null : g.apply(x);
    }

(obviously with better names) and then:

            table.addCell(ifNotNull(totalStats, CommonStats::getSearch, s -> s.getTotal().getSuggestCount()));
            table.addCell(ifNotNull(primaryStats, CommonStats::getSearch, s -> s.getTotal().getSuggestCount()));

at the call sites. It's not clear to me whether or not this is an improvement. Personally I think it's fine as-is, we can keep it simple here. It might be easier to read and maintain. What do you think?

dnhatn added a commit that referenced this pull request Oct 10, 2017
When a node which contains the primary shard is unavailable, the primary
stats (and the total stats) of an `IndexStats` will be empty for a short
moment (while the primary shard is being relocated). However, we assume
that these stats are always non-empty when handling `_cat/indices` in
RestIndicesAction. This commit checks the content of these stats before
accessing.

Closes #26942
@dnhatn
Copy link
Member Author

dnhatn commented Oct 10, 2017

@jasontedor, I was trying a slightly different but similar approach to your proposal. However, I found that it was a bit harder to read than null-checks.

@jasontedor
Copy link
Member

However, I found that it was a bit harder to read than null-checks.

Yeah, that's what I suspected would be the case.

dnhatn added a commit that referenced this pull request Oct 10, 2017
When a node which contains the primary shard is unavailable, the primary
stats (and the total stats) of an `IndexStats` will be empty for a short
moment (while the primary shard is being relocated). However, we assume
that these stats are always non-empty when handling `_cat/indices` in
RestIndicesAction. This commit checks the content of these stats before
accessing.

Closes #26942
dnhatn added a commit that referenced this pull request Oct 10, 2017
When a node which contains the primary shard is unavailable, the primary
stats (and the total stats) of an `IndexStats` will be empty for a short
moment (while the primary shard is being relocated). However, we assume
that these stats are always non-empty when handling `_cat/indices` in
RestIndicesAction. This commit checks the content of these stats before
accessing.

Closes #26942
@dnhatn dnhatn deleted the npe-cat-indices branch October 10, 2017 21:42
jasontedor added a commit that referenced this pull request Oct 12, 2017
* master: (35 commits)
  Create weights lazily in filter and filters aggregation (#26983)
  Use a dedicated ThreadGroup in rest sniffer (#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (#26841)
  Cat shards bytes (#26952)
  Add support for parsing inline script (#23824) (#26846)
  Change default value to true for transpositions parameter of fuzzy query (#26901)
  Adding unreleased 5.6.4 version number to Version.java
  Rename TCPTransportTests to TcpTransportTests (#26954)
  Fix NPE for /_cat/indices when no primary shard (#26953)
  [DOCS] Fixed indentation of the definition list.
  Fix formatting in channel close test
  Check for closed connection while opening
  Clarify systemd overrides
  [DOCS] Plugin Installation for Windows (#21671)
  Painless: add tests for cached boxing (#24163)
  Don't detect source's XContentType in DocumentParser.parseDocument() (#26880)
  Fix handling of paths containing parentheses
  Allow only a fixed-size receive predictor (#26165)
  Add Homebrew instructions to getting started
  ...
jasontedor added a commit that referenced this pull request Oct 12, 2017
* 6.x: (32 commits)
  Use a dedicated ThreadGroup in rest sniffer (#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (#26841)
  Cat shards bytes (#26952)
  Adding unreleased 5.6.4 version number to Version.java
  Rename TCPTransportTests to TcpTransportTests (#26954)
  Fix NPE for /_cat/indices when no primary shard (#26953)
  [DOCS] Fixed indentation of the definition list.
  Check for closed connection while opening
  Clarify systemd overrides
  [DOCS] Plugin Installation for Windows (#21671)
  Painless: add tests for cached boxing (#24163)
  Don't detect source's XContentType in DocumentParser.parseDocument() (#26880)
  Return List instead of an array from settings (#26903)
  Emit deprecation warning for variable size predictor
  Fix handling of paths containing parentheses
  Deprecate variable-size receive predictor
  Add Homebrew instructions to getting started
  ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number
  Scripting: Fix expressions to temporarily support filter scripts (#26824)
  ...
@lcawl lcawl added v6.0.0-rc2 and removed v6.0.0 labels Oct 30, 2017
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@clintongormley clintongormley added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. and removed :Cluster labels Feb 13, 2018
rahulanishetty pushed a commit to rahulanishetty/elasticsearch that referenced this pull request Jul 26, 2018
When a node which contains the primary shard is unavailable, the primary
stats (and the total stats) of an `IndexStats` will be empty for a short
moment (while the primary shard is being relocated). However, we assume
that these stats are always non-empty when handling `_cat/indices` in
RestIndicesAction. This commit checks the content of these stats before
accessing.

Closes elastic#26942
@jpountz jpountz removed the :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. label Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants