Skip to content

Conversation

@gbbafna
Copy link
Collaborator

@gbbafna gbbafna commented Jul 25, 2022

Signed-off-by: Gaurav Bafna gbbafna@amazon.com

Description

Before starting the replication , we need to validate for correctness of index settings. This would throw validation exception before starting replication itself.

If not done, the snapshot restore phase of replication will fail asynchronously and customer would need to stop and start the replication again without error message being communicated .

Issues Resolved

#460
#298

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • [] New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
@gbbafna gbbafna requested review from a team, saikaranam-amazon and soosinha July 25, 2022 07:16
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022

Codecov Report

Merging #461 (9b67310) into main (27898e2) will decrease coverage by 1.97%.
The diff coverage is 100.00%.

❗ Current head 9b67310 differs from pull request most recent head afb9fbb. Consider uploading reports for the commit afb9fbb to get more accurate results

@@             Coverage Diff              @@
##               main     #461      +/-   ##
============================================
- Coverage     74.52%   72.54%   -1.98%     
+ Complexity     1002      985      -17     
============================================
  Files           141      141              
  Lines          4565     4579      +14     
  Branches        504      506       +2     
============================================
- Hits           3402     3322      -80     
- Misses          847      937      +90     
- Partials        316      320       +4     
Impacted Files Coverage Δ
...tion/action/index/TransportReplicateIndexAction.kt 76.66% <100.00%> (+2.59%) ⬆️
.../org/opensearch/replication/util/ValidationUtil.kt 62.74% <100.00%> (+6.93%) ⬆️
...cation/action/changes/TransportGetChangesAction.kt 63.49% <0.00%> (-20.64%) ⬇️
...ch/replication/task/index/IndexReplicationState.kt 51.66% <0.00%> (-15.00%) ⬇️
...ication/action/setup/TransportSetupChecksAction.kt 59.52% <0.00%> (-14.29%) ⬇️
...ication/seqno/RemoteClusterRetentionLeaseHelper.kt 80.00% <0.00%> (-11.67%) ⬇️
...tion/repository/RemoteClusterMultiChunkTransfer.kt 90.24% <0.00%> (-9.76%) ⬇️
.../replication/metadata/store/ReplicationMetadata.kt 62.96% <0.00%> (-8.65%) ⬇️
...action/stop/TransportStopIndexReplicationAction.kt 68.00% <0.00%> (-6.67%) ⬇️
...ication/action/stop/StopIndexReplicationRequest.kt 50.00% <0.00%> (-5.00%) ⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

soosinha
soosinha previously approved these changes Jul 25, 2022
.build()

assertThatThrownBy {
followerClient.startReplication(StartReplicationRequest("source", leaderIndexName, followerIndexName, settings = settings))
Copy link
Member

Choose a reason for hiding this comment

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

Can we validated status code as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done .

overriddenSettings: Settings,
metadataCreateIndexService: MetadataCreateIndexService
) {
val settingsList = arrayOf(leaderSettings, overriddenSettings)
Copy link
Member

Choose a reason for hiding this comment

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

Please validate to see if the behavior is as expected, due to open bug on replication side: #298?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have fixed this bug in this PR as well. Now we are retrieving the default settings as well .

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
@gbbafna gbbafna merged commit 93b43f4 into opensearch-project:main Aug 3, 2022
@gbbafna gbbafna deleted the setting-validate branch August 3, 2022 11:30
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 3, 2022
* Adding Index Settings validation before starting replication

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

* Retrieving default index settings before starting replication

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
(cherry picked from commit 93b43f4)
gbbafna added a commit that referenced this pull request Aug 3, 2022
…470)

* Adding Index Settings validation before starting replication

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

* Retrieving default index settings before starting replication

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
(cherry picked from commit 93b43f4)

Co-authored-by: Gaurav Bafna <gbbafna@amazon.com>
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.

4 participants