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

Replication namespace #464

Merged
merged 81 commits into from
Nov 18, 2024
Merged

Conversation

Tokesh
Copy link
Collaborator

@Tokesh Tokesh commented Aug 3, 2024

Description

adding replication namespace specs

Issues Resolved

[#232]

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.

Tokesh and others added 4 commits August 3, 2024 22:24
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Niyazbek Torekeldi <78027392+Tokesh@users.noreply.github.com>
Copy link
Contributor

github-actions bot commented Aug 3, 2024

Changes Analysis

Commit SHA: 4629754
Comparing To SHA: bf35601

API Changes

Summary

├─┬Paths
│ ├──[➕] path (5480:3)
│ ├──[➕] path (5498:3)
│ ├──[➕] path (5460:3)
│ ├──[➕] path (5440:3)
│ ├──[➕] path (5518:3)
│ ├──[➕] path (5555:3)
│ ├──[➕] path (5383:3)
│ ├──[➕] path (5538:3)
│ ├──[➕] path (5420:3)
│ └──[➕] path (5572:3)
└─┬Components
  ├──[➕] requestBodies (27120:7)
  ├──[➕] requestBodies (27114:7)
  ├──[➕] requestBodies (27108:7)
  ├──[➕] requestBodies (27102:7)
  ├──[➕] requestBodies (27090:7)
  ├──[➕] requestBodies (27096:7)
  ├──[➕] requestBodies (27126:7)
  ├──[➕] responses (30234:7)
  ├──[➕] responses (30228:7)
  ├──[➕] responses (30246:7)
  ├──[➕] responses (30216:7)
  ├──[➕] responses (30210:7)
  ├──[➕] responses (30252:7)
  ├──[➕] responses (30204:7)
  ├──[➕] responses (30240:7)
  ├──[➕] responses (30264:7)
  ├──[➕] responses (30222:7)
  ├──[➕] responses (30258:7)
  ├──[➕] parameters (23710:7)
  ├──[➕] parameters (23750:7)
  ├──[➕] parameters (23718:7)
  ├──[➕] parameters (23726:7)
  ├──[➕] parameters (23734:7)
  ├──[➕] parameters (23742:7)
  ├──[➕] schemas (55615:7)
  ├──[➕] schemas (55483:7)
  ├──[➕] schemas (55447:7)
  ├──[➕] schemas (55466:7)
  ├──[➕] schemas (55494:7)
  ├──[➕] schemas (55606:7)
  ├──[➕] schemas (55650:7)
  ├──[➕] schemas (55620:7)
  ├──[➕] schemas (55501:7)
  ├──[➕] schemas (55538:7)
  ├──[➕] schemas (55566:7)
  ├──[➕] schemas (55583:7)
  ├──[➕] schemas (55662:7)
  ├──[➕] schemas (55641:7)
  └──[➕] schemas (55559:7)

Document Element Total Changes Breaking Changes
paths 10 0
components 39 0
  • Total Changes: 49
  • Additions: 49

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11893879787/artifacts/2201699624

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 599 (58.67 %) 11 (1.08 %)
Uncovered (%) 433 (42.41 %) 422 (41.33 %) -11 (-1.08 %)
Unknown 42 42 0

Tokesh added 6 commits August 4, 2024 01:43
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
@dblock
Copy link
Member

dblock commented Aug 3, 2024

Should we split the docker-compose.yml into docker-compose-default, docker-compose-multinode.yml or something like that?

@Tokesh
Copy link
Collaborator Author

Tokesh commented Aug 3, 2024

Should we split the docker-compose.yml into docker-compose-default, docker-compose-multinode.yml or something like that?

wow, do you work even on weekends?
Yes, I encountered this today, tested it on a local machine. Now I was thinking of asking whether it is worth separating or inventing some flags.

@dblock
Copy link
Member

dblock commented Aug 7, 2024

Take a look at #472 that allows custom docker compose and tests.

@Tokesh
Copy link
Collaborator Author

Tokesh commented Aug 7, 2024

Take a look at #472 that allows custom docker compose and tests.

Thank you! It looks really good
I'll wait for your PR to merge so I can continue working on the replication namespace.

@dblock
Copy link
Member

dblock commented Aug 13, 2024

Take a look at #472 that allows custom docker compose and tests.

Thank you! It looks really good I'll wait for your PR to merge so I can continue working on the replication namespace.

@Tokesh that got merged FYI

Copy link
Contributor

github-actions bot commented Aug 14, 2024

Spec Test Coverage Analysis

Total Tested
528 359 (67.99 %)

Tokesh added 7 commits August 14, 2024 23:42
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
@Tokesh
Copy link
Collaborator Author

Tokesh commented Nov 10, 2024

@Tokesh want to finish this?

I passed the biggest part of CI/CD.
Any suggestions to refactor this code?

Signed-off-by: Tokesh <tokesh789@gmail.com>
dblock
dblock previously approved these changes Nov 11, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This looks great!

I added .yml files to Vale preprocessing in #660, let's rebase this once it's merged to make sure it worked.

.github/workflows/test-spec.yml Outdated Show resolved Hide resolved
tests/plugins/replication/replica.yaml Outdated Show resolved Hide resolved
index.number_of_replicas: 2
response:
status: 200
- synopsis: Create replication rule.
Copy link
Member

Choose a reason for hiding this comment

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

Extract autofollow into a separate file called autofollow.yaml.

@@ -0,0 +1,127 @@
$schema: ../../../json_schemas/test_story.schema.yaml
Copy link
Member

Choose a reason for hiding this comment

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

There are multiple examples here surrounding replication, one that starts replication, so see if that can be extracted in a start.yaml, and another that gets replication stats, belongs in stats.yaml.

Basically think of how these examples could be used in various parts of the documentation on their own, even if it means duplicating some API calls in prologues and epilogues.

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 think i can divide to:
replica.yaml
stats.yaml
autofollow.yaml
replica-rules.yaml
And the code will be duplicated everywhere where we create an index and start replicating it, if it works, then I can divide it like this and do it in a few days.

Copy link
Member

@dblock dblock Nov 11, 2024

Choose a reason for hiding this comment

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

That works. Maybe rename replica.yaml to index.yaml, that one will have the replication as the main act.

Generally these files match the namespace and/or the API path, not the "theme", so replica-rules is not a thing, it will need another name.

Signed-off-by: Tokesh <tokesh789@gmail.com>
Tokesh and others added 5 commits November 17, 2024 17:36
Signed-off-by: Niyazbek Torekeldi <78027392+Tokesh@users.noreply.github.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
@Tokesh
Copy link
Collaborator Author

Tokesh commented Nov 17, 2024

Ready for your review! @dblock @nhtruong

@Tokesh Tokesh requested a review from dblock November 17, 2024 13:23
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Glad we have a passing build!

Go through the fields and check whether they are semantically meaningful and need to be a $ref to something that exists (time values, index names, etc.).

.github/workflows/test-spec.yml Outdated Show resolved Hide resolved
spec/namespaces/replication.yaml Outdated Show resolved Hide resolved
spec/schemas/replication._common.yaml Show resolved Hide resolved
spec/schemas/replication._common.yaml Outdated Show resolved Hide resolved
spec/schemas/replication._common.yaml Outdated Show resolved Hide resolved
spec/schemas/replication._common.yaml Show resolved Hide resolved
spec/schemas/replication._common.yaml Show resolved Hide resolved
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
Signed-off-by: Tokesh <tokesh789@gmail.com>
@Tokesh Tokesh requested a review from dblock November 17, 2024 20:00
@Tokesh
Copy link
Collaborator Author

Tokesh commented Nov 17, 2024

Thank you for a explained review. I read _common schemes and tried to fix all the places, where i can implement them.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Small one left, the rest LGTM!

type: object
properties:
settings:
anyOf:
Copy link
Member

Choose a reason for hiding this comment

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

Is this a combination of SettingsBody and these? So should be allOf.

Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock merged commit 4771bfd into opensearch-project:main Nov 18, 2024
26 checks passed
@dblock
Copy link
Member

dblock commented Nov 18, 2024

I made the change I suggested above and merged, thank you, this is great work @Tokesh!

@Tokesh
Copy link
Collaborator Author

Tokesh commented Nov 19, 2024

Oh, i just checked. Thank you so much for detailed reviews and help! It's a pleasure working with you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants