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

Simpler using compressed oops flag representation #15509

Merged
merged 2 commits into from Dec 17, 2015

Conversation

Projects
None yet
2 participants
@jasontedor
Copy link
Member

commented Dec 17, 2015

This commit modifies the internal representation of the JVM flag
UseCompressedOops to just be a String. This means we can just store the
value of the flag or "unknown" directly so that we do not have to engage
in shenanigans with three-valued logic around a boxed boolean.

Relates #15489

Simpler using compressed oops flag representation
This commit modifies the internal representation of the JVM flag
UseCompressedOops to just be a String. This means we can just store the
value of the flag or "unknown" directly so that we do not have to engage
in shenanigans with three-valued logic around a boxed boolean.

Relates #15489
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2015

The ultimate endgame here is simpler handling in the backwards compatibility code in the 2.x line so that we can distinguish between "true", "false" and "unknown" from a >= 2.2.0 node, and null from a < 2.2.0 node.

} else {
usingCompressedOops = null;
}
useCompressedOops = in.readOptionalString();

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 17, 2015

Contributor

Can it be null?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 17, 2015

Author Member

@nik9000 Not in master, good catch. I'll remove the optional for master but it will be needed on the 2.x backport.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2015

LGTM - I left a comment asking about the return value from the jvm's info API. Maybe if it returns null we should set it to "unknown".

@@ -307,7 +314,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.field(Fields.GC_COLLECTORS, gcCollectors);
builder.field(Fields.MEMORY_POOLS, memoryPools);

builder.field(Fields.USING_COMPRESSED_OOPS, usingCompressedOops == null ? "unknown" : Boolean.toString(usingCompressedOops));
builder.field(Fields.USING_COMPRESSED_OOPS, useCompressedOops);

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 17, 2015

Contributor

If it can be null we should skip it here.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 17, 2015

Author Member

@nik9000 Yes, that's exactly how it will be on the backport but it's not needed here.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2015

LGTM - I left a comment asking about the return value from the jvm's info API. Maybe if it returns null we should set it to "unknown".

The return from the API can not be null.

jasontedor added a commit that referenced this pull request Dec 17, 2015

Merge pull request #15509 from jasontedor/simpler-compressed-oops-rep…
…resentation

Simpler using compressed oops flag representation

@jasontedor jasontedor merged commit c867c7c into elastic:master Dec 17, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jasontedor jasontedor deleted the jasontedor:simpler-compressed-oops-representation branch Dec 17, 2015

jasontedor added a commit that referenced this pull request Dec 17, 2015

Simpler using compressed oops flag representation
This commit backports commit c867c7c
from master to 2.x.

Relates #15509
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2015

Thank you for reviewing @nik9000.

@jasontedor jasontedor removed the review label Dec 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.