Skip to content

Test/107515 RestoreTemplateWithMatchOnlyTextMapperIT #120392

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

Conversation

drempapis
Copy link
Contributor

Closes #107515

I observed that when comparing the cluster states, local and master were equal, but the order was different in some cases. Converting to a byte array and getting the length can introduce junk data. Alternatively, the comparison can be made by checking if the returned state strings are anagrams, they contain the same characters.

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 labels Jan 17, 2025
@drempapis drempapis requested a review from gmarouli January 17, 2025 14:51
@drempapis drempapis self-assigned this Jan 17, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @drempapis, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@drempapis drempapis added auto-backport Automatically create backport pull requests when merged v8.18.0 labels Jan 17, 2025
@drempapis drempapis changed the title Test/107515 restore template with match only text mapper it fail Test/107515 rRestoreTemplateWithMatchOnlyTextMapperIT Jan 17, 2025
@drempapis drempapis changed the title Test/107515 rRestoreTemplateWithMatchOnlyTextMapperIT Test/107515 RestoreTemplateWithMatchOnlyTextMapperIT Jan 17, 2025
@drempapis drempapis requested a review from a team January 22, 2025 08:18
@drempapis
Copy link
Contributor Author

@elastic/es-core-infra, can you please have a look to the provided solution?

@drempapis drempapis removed the request for review from gmarouli January 22, 2025 08:44
@thecoop
Copy link
Member

thecoop commented Jan 22, 2025

Can you give an example of how the sizes could be different but the content is the same? (it's unclear from the linked issue exactly what the difference is)

@drempapis
Copy link
Contributor Author

drempapis commented Jan 22, 2025

I found it debugging the failing test case. The string representation of the cluster state was the following for both masterClusterState and localClusterStatebut in a different order. The calculated byte sizes were different.

"index_template":{
   "index_template":{
      "t1":{
         "index_patterns":[
            "test-index-*"
         ],
         "template":{
            "mappings":{
               "properties":{
                  "@timestamp":{
                     "format":"date_optional_time||epoch_millis",
                     "type":"date"
                  },
                  "flag":{
                     "type":"boolean"
                  },
                  "message":{
                     "type":"match_only_text"
                  }
               }
            }
         },
         "composed_of":[
            
         ],
         "data_stream":{
            "hidden":false,
            "allow_custom_routing":false
         }
      }
   }
}"index-graveyard":"IndexGraveyard"[
   [
      [
         "index="[
            test-idx-no-ds/xNBvevzTQ5CzuUFxnzjj-g
         ],
         "deleteDate=2025-01-17T13":"44":31.746Z
      ]
   ]
]"persistent_tasks":{
   "last_allocation_id":3,
   "tasks":[[89, 83, 85, 73, 84, 69, 45, 84, 69, 83, 84, 95, 87, 79, 82, 75, 69, 82, 95, 86, 77, 61, 91, 51, 48, 93, 45, 67, 76, 85, 83, 84, 69, 82, 95, 83, 69, 69, 68, 61, 91, 45, 51, 51, 54, 55, 53, 53, 52, 57, 53, 51, 52, 57, 52, 56, 51, 55, 54, 54, 57, 93, 45, 72, 65, 83, 72, 61, 91, 49, 51, 69, 69, 68, 51, 56, 49, 69, 56, 52, 55, 93, 45, 99, 108, 117, 115, 116, 101, 114, 0, 0, 0, 0, 0, 0, 0, 28, 22, 99, +4,464 more]
      {
         "id":"health-node",
         "task":{
            "health-node":{
               "params":{
                  
               }
            }
         },
         "allocation_id":3,
         "assignment":{
            "executor_node":"XcbfjiTHRWqhLHWvaWJOvA",
            "explanation":""
         },
         "allocation_id_on_last_status_update":0
      }
   ]
}"repositories":{
   "repo":{
      "type":"fs",
      "uuid":"N3yt0VE5S1aVe7VQgx5tkw",
      "settings":{
         "location":"/Users/dimitrisrempapis/workspace/elasticsearch/modules/data-streams/build/testrun/internalClusterTest/temp/org.elasticsearch.datastreams.RestoreTemplateWithMatchOnlyTextMapperIT_460BA435414BE7D1-001/tempDir-002/repos/IcxztPjmUe"
      },
      "generation":0,
      "pending_generation":0
   }
}"data_stream":{
   "data_stream":{
      
   },
   "data_stream_aliases":{
      
   }
}

@thecoop
Copy link
Member

thecoop commented Jan 22, 2025

So the main way to check that two xcontent things are the same is to convert them to maps, and check the map equality. That way it doesnt consider order. There's other methods on XContentTestUtils that help with equality checks too

@drempapis
Copy link
Contributor Author

Thank you, @thecoop, for the feedback. Do you want me to update this pr using the 'XContentTestUtils` or to assign it to core-infra for a second look?

@thecoop
Copy link
Member

thecoop commented Jan 22, 2025

Could you update it to use XContentTestUtils (you could copy the equality checks used by ensureClusterStateCanBeReadByNodeTool for example)? If it still doesnt pass, then assign it to us to take a closer look.

assertEquals("cluster state size does not match", masterClusterStateSize, localClusterStateSize);

// Compare the steMaps for equality.
assertNull(XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder(masterStateMap, localStateMap));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thecoop, I updated the code using the XContentTestUtils to compare the stateMaps. Please review the proposed solution

Copy link
Member

@thecoop thecoop left a comment

Choose a reason for hiding this comment

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

LGTM

@drempapis drempapis merged commit 66db8c7 into elastic:main Jan 24, 2025
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 120392

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged backport pending >bug :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] RestoreTemplateWithMatchOnlyTextMapperIT test failing
3 participants