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

[Feature/extensions] Fish out Create Detector and remove everything else for extensions. #623

Merged
merged 11 commits into from
Aug 15, 2022
Merged

[Feature/extensions] Fish out Create Detector and remove everything else for extensions. #623

merged 11 commits into from
Aug 15, 2022

Conversation

saratvemulapalli
Copy link
Member

@saratvemulapalli saratvemulapalli commented Aug 2, 2022

Description

Commenting out code for all features of AD except create detector.

I've manually tested and create detector still works.

Manual Test

  1. Create Index
curl -XPUT localhost:9200/server_log --data '{
"mappings" : {
    "_default_":{
        "_timestamp" : {
            "enabled" : true,
            "store" : true
        }
    }
  }
}' -H "Content-Type:application/json"
  1. Ingest a doc
curl -XPUT "localhost:9200/server_log/_doc/1?pretty" --data '{"date": "2015-01-01"}' -H "Content-Type:application/json"
{
  "_index" : "server_log",
  "_id" : "1",
  "_version" : 1,
  "result" : "created",
  "_shards" : {
    "total" : 2,
    "successful" : 1,
    "failed" : 0
  },
  "_seq_no" : 0,
  "_primary_term" : 1
}
  1. Create Detector
curl -XPOST "localhost:9200/_plugins/_anomaly_detection/detectors?pretty" -H "Content-Type:application/json" --data '{
"name":"test-detector", "time_field":"date", "indices": [ "server_log*" ]
}'
{
  "_id" : "XinDW4IBUCK3JmVb3oQ0",
  "_version" : 1,
  "_seq_no" : 0,
  "anomaly_detector" : {
    "name" : "test-detector",
    "description" : "",
    "time_field" : "date",
    "indices" : [
      "server_log*"
    ],
    "filter_query" : {
      "match_all" : {
        "boost" : 1.0
      }
    },
    "detection_interval" : {
      "period" : {
        "interval" : 10,
        "unit" : "Minutes"
      }
    },
    "window_delay" : {
      "period" : {
        "interval" : 0,
        "unit" : "Seconds"
      }
    },
    "shingle_size" : 8,
    "schema_version" : 0,
    "feature_attributes" : [ ],
    "last_update_time" : 1659396939315,
    "detector_type" : "SINGLE_ENTITY"
  },
  "_primary_term" : 1
}

Note I'll mark the PR in draft state to comment out the tests. Obviously a lot of tests would fail :)
Everything works!!

Issues Resolved

Closes opensearch-project/opensearch-sdk-java#20

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: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
Signed-off-by: Sarat Vemulapalli <vemulapallisarat@gmail.com>
@saratvemulapalli saratvemulapalli marked this pull request as ready for review August 2, 2022 05:46
@saratvemulapalli saratvemulapalli requested review from a team and joshpalis August 2, 2022 05:46
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM although it's hard to review a negative diff. Makes me wonder if we shouldn't just start with a fresh repo and bring things over that we want to keep.

@saratvemulapalli
Copy link
Member Author

Looking for one more pair of eyes.
@owaiskazi19 @joshpalis @ryanbogan if you guys have some bandwidth.

@owaiskazi19
Copy link
Member

owaiskazi19 commented Aug 5, 2022

LGTM although it's hard to review a negative diff. Makes me wonder if we shouldn't just start with a fresh repo and bring things over that we want to keep.

+1 to this. @saratvemulapalli WDYT?

@owaiskazi19
Copy link
Member

Should we merge #619 before this?

@saratvemulapalli
Copy link
Member Author

@dbwiddis @owaiskazi19 The problem maintain a fresh piece of code is that we have to keep understanding the diff between main and feature/extensions.
Its going to be hard to track them, commented code will have everything in place and merging will be easy down the line.

I would propose keeping this branch with all commented code and when we start integrating the SDK we can pull the fresh pieces of code. What do you guys think?

@owaiskazi19
Copy link
Member

@dbwiddis @owaiskazi19 The problem maintain a fresh piece of code is that we have to keep understanding the diff between main and feature/extensions. Its going to be hard to track them, commented code will have everything in place and merging will be easy down the line.

I would propose keeping this branch with all commented code and when we start integrating the SDK we can pull the fresh pieces of code. What do you guys think?

Makes sense. Thanks for showing the big picture here.

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @saratvemulapalli . I don't have the green 😞

@saratvemulapalli
Copy link
Member Author

LGTM! Thanks @saratvemulapalli . I don't have the green 😞

Well more commits will earn you the green :)

@saratvemulapalli
Copy link
Member Author

Should we merge #619 before this?

Thats a good point.
Sure let me review those changes. I'll wait until the PR is merged.

@saratvemulapalli saratvemulapalli merged commit fc3e7ca into opensearch-project:feature/extensions Aug 15, 2022
@saratvemulapalli saratvemulapalli deleted the create-detector-feature-extensions branch August 15, 2022 17:12
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.

3 participants