Skip to content

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

Merged
dnhatn merged 1 commit intoelastic:masterfrom
dnhatn:npe-cat-indices
Oct 10, 2017
Merged

Fix NPE for /_cat/indices when no primary shard#26953
dnhatn merged 1 commit intoelastic:masterfrom
dnhatn:npe-cat-indices

Conversation

@dnhatn
Copy link
Copy Markdown
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

@dnhatn
Fix NPE for /_cat/indices when no primary shard
cd9955a 
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 dnhatn requested a review from jasontedor October 10, 2017 19:36
@dnhatn
Copy link
Copy Markdown
Member Author

dnhatn commented Oct 10, 2017 •
edited
Loading

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

jasontedor
jasontedor approved these changes Oct 10, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

jasontedor commented Oct 10, 2017

@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
@dnhatn
Fix NPE for /_cat/indices when no primary shard (#26953)
d3aea0b 
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
Copy Markdown
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
Copy Markdown
Member

jasontedor commented Oct 10, 2017

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
@dnhatn
Fix NPE for /_cat/indices when no primary shard (#26953)
be21038 
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
@dnhatn
Fix NPE for /_cat/indices when no primary shard (#26953)
a37d949 
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
@jasontedor
Merge branch 'master' into ccr
7c1bcfc 
* 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
@jasontedor
Merge branch '6.x' into ccr-6.x
b6227db 
* 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)
  ...
@tvernum tvernum mentioned this pull request Oct 19, 2017
Closed
@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/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
@dnhatn
Fix NPE for /_cat/indices when no primary shard (elastic#26953)
d860bd3 
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/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

Reviewers

@jasontedor jasontedor jasontedor approved these changes

Assignees

No one assigned

Labels

>bug :Core/Infra/CAT APIs Text APIs behind /_cat v5.6.4 v6.0.0-rc2 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dnhatn @jasontedor @clintongormley @colings86 @jpountz @lcawl