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.

Sign up for GitHub

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

Improve test times for tests using RandomObjects::addFields #31556

Merged
merged 2 commits into from
Jun 26, 2018
Merged

Improve test times for tests using RandomObjects::addFields #31556

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2b4f08
improve Randomobjects::addfield logic
sohaibiftikhar Jun 25, 2018
81c1645
simplified addFields
sohaibiftikhar Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentVerboseResultTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
public class SimulateDocumentVerboseResultTests extends AbstractXContentTestCase<SimulateDocumentVerboseResult> {

static SimulateDocumentVerboseResult createTestInstance(boolean withFailures) {
int numDocs = randomIntBetween(0, 10);
int numDocs = randomIntBetween(0, 5);
List<SimulateProcessorResult> results = new ArrayList<>();
for (int i = 0; i<numDocs; i++) {
boolean isSuccessful = !(withFailures && randomBoolean());
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public void testSerialization() throws IOException {
}

static SimulatePipelineResponse createInstance(String pipelineId, boolean isVerbose, boolean withFailure) {
int numResults = randomIntBetween(1, 10);
int numResults = randomIntBetween(1, 5);
List<SimulateDocumentResult> results = new ArrayList<>(numResults);
for (int i = 0; i < numResults; i++) {
if (isVerbose) {
Expand Down
23 changes: 16 additions & 7 deletions test/framework/src/main/java/org/elasticsearch/test/RandomObjects.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public static BytesReference randomSource(Random random, XContentType xContentTy
public static BytesReference randomSource(Random random, XContentType xContentType, int minNumFields) {
try (XContentBuilder builder = XContentFactory.contentBuilder(xContentType)) {
builder.startObject();
addFields(random, builder, minNumFields, 0);
addFields(random, builder, minNumFields, 0, 0);
builder.endObject();
return BytesReference.bytes(builder);
} catch(IOException e) {
Expand All @@ -185,14 +185,21 @@ public static BytesReference randomSource(Random random, XContentType xContentTy

/**
* Randomly adds fields, objects, or arrays to the provided builder. The maximum depth is 5.
* @return Returns the number of fields in the document
*/
private static void addFields(Random random, XContentBuilder builder, int minNumFields, int currentDepth) throws IOException {
int numFields = randomIntBetween(random, minNumFields, 10);
private static int addFields(Random random, XContentBuilder builder, int minNumFields, int currentDepth,
int currentFields) throws IOException {
int maxTotalFields = 200;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what value is good for this. From my results, 200 seemed to give decently big documents without going beyond 100KB.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we need such big docs, 100kb seems a lot still even if it's the upper bound.

int maxFields = Math.max(minNumFields, 10 - (currentFields * 10)/maxTotalFields); // Map to range 0-10
int numFields = randomIntBetween(random, minNumFields, maxFields);
int maxDepth = 5 - (currentFields * 5)/maxTotalFields; // Map to range 0-5
currentFields += numFields;
Copy link
Member

Choose a reason for hiding this comment

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

with the goal of trying to keep things simple, I wonder if decreasing the max number of fields e.g. to 5 would be enough. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be. Will need to test this for the average case. The potentially maximum number of fields would still be more than 3000.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we should also decrease the chance that we add another child object compared to leaf fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't quite understand. We, already have a check on the depth which takes care of this? Or do you mean something else?

Copy link
Member

Choose a reason for hiding this comment

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

yes we have a check, but I think we end up adding more objects and arrays than we need.

if (currentDepth < 5 && random.nextBoolean()) {

the above if (taken from addFields method) means that we add a new object or array if maxDepth allows, one out of two times. Maybe we want to lower that probability?

Copy link
Contributor Author

@sohaibiftikhar sohaibiftikhar Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah okay. I will try to lower the probability based on current depth. What did you mean by leaf fields? The neighbours?

Copy link
Member

Choose a reason for hiding this comment

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

you can also do something similar to what LuceneTestCase#rarely does. we can do it regardless depth to keep things simple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javanna I simplified it now. So that max fields is 5 and the probability is 30%. It seems to give similar results in terms of timing.

for (int i = 0; i < numFields; i++) {
if (currentDepth < 5 && random.nextBoolean()) {
if (currentDepth < maxDepth && random.nextBoolean()) {
if (random.nextBoolean()) {
builder.startObject(RandomStrings.randomAsciiOfLengthBetween(random, 6, 10));
addFields(random, builder, minNumFields, currentDepth + 1);
currentFields =
addFields(random, builder, minNumFields, currentDepth + 1, currentFields);
builder.endObject();
} else {
builder.startArray(RandomStrings.randomAsciiOfLengthBetween(random, 6, 10));
Expand All @@ -205,7 +212,8 @@ private static void addFields(Random random, XContentBuilder builder, int minNum
for (int j = 0; j < numElements; j++) {
if (object) {
builder.startObject();
addFields(random, builder, minNumFields, 5);
currentFields =
addFields(random, builder, minNumFields, 5, currentFields);
builder.endObject();
} else {
builder.value(randomFieldValue(random, dataType));
Expand All @@ -215,9 +223,10 @@ private static void addFields(Random random, XContentBuilder builder, int minNum
}
} else {
builder.field(RandomStrings.randomAsciiOfLengthBetween(random, 6, 10),
randomFieldValue(random, randomDataType(random)));
randomFieldValue(random, randomDataType(random)));
}
}
return currentFields;
}

private static int randomDataType(Random random) {
Expand Down