-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix/elastic store update #5210
base: master
Are you sure you want to change the base?
Fix/elastic store update #5210
Changes from all commits
75318f0
f3690a0
a2dacf7
23bb9df
be2c101
5707726
51e8ab2
3799f49
4d1090d
08fc4a3
f50c70d
cc08255
d309986
0676879
ab12b5b
a742486
74b14c5
97734cb
db8bad0
87b3dbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# This workflow uses actions that are not certified by GitHub. | ||
# They are provided by a third-party and are governed by | ||
# separate terms of service, privacy policy, and support | ||
# documentation. | ||
|
||
name: Java CI | ||
|
||
on: | ||
workflow_dispatch: | ||
|
||
jobs: | ||
build: | ||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- uses: actions/checkout@v2 | ||
|
||
- name: Login to github docker Registry | ||
run: docker login ghcr.io -u ${{ github.actor }} -p ${{ secrets.GITHUB_TOKEN }} | ||
|
||
- name: Set the image tag | ||
id: image_vars | ||
run: | | ||
TAG=latest | ||
echo ::set-output name=tag::$(echo $TAG | awk '{print tolower($0)}') | ||
|
||
- uses: actions/setup-java@v2 | ||
with: | ||
distribution: adopt | ||
java-version: 11 | ||
|
||
- name: Setup Gradle | ||
uses: gradle/gradle-build-action@v2 | ||
|
||
- name: Execute Gradle build | ||
run: ./gradlew distDocker -PdockerRegistry=ghcr.io -PdockerImagePrefix=${{ github.repository }} -PdockerImageTag=${{steps.image_vars.outputs.tag}} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,8 +89,8 @@ dependencies { | |
compile "io.reactivex:rxjava:1.3.8" | ||
compile "io.reactivex:rxjava-reactive-streams:1.2.1" | ||
compile "com.microsoft.azure:azure-cosmosdb:2.6.2" | ||
compile "com.sksamuel.elastic4s:elastic4s-client-esjava_${gradle.scala.depVersion}:7.10.3" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this compatible with elasticsearch 6.x version? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked that 6.x is not compatible with this PR because 6.x requires a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concerned about the license. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What wouldn't they support? Elastic changed the license model sometime after 7.10.x, so this is still entirely covered Apache 2.0. It's also worth mentioning we don't have to use Elasticsearch. This update makes it possible to use Opensearch, which is covered entirely by Apache 2.0. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new scheduler relies on multiple filtered aliases against one ES index. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to tell you, I've been using OpenWhisk with my ES 7.10 (Opensearch) cluster for the last week without any issues... For what it's worth I'm working on updating the tests so they work correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you using FPCScheduler too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I don't think I am. That said, I can't find anything in the Elasticsearch documentation or release notes that suggests "multiple filtered aliases against one ES index" would have been removed. Was it something you read that gave you cause for concern? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @style95 I think new scheduler doen't rely on we are using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm.. |
||
|
||
compile "com.sksamuel.elastic4s:elastic4s-http_${gradle.scala.depVersion}:6.7.4" | ||
//for mongo | ||
compile "org.mongodb.scala:mongo-scala-driver_${gradle.scala.depVersion}:2.7.0" | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the goal here to run GHA instead of using Travis?
In any case this change seems orthogonal to the rest of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created that because I don't have a Travis or Jenkins environment to build this in and my personal laptop has an M1 chip in it. Additionally, I wanted to be sure that I had fixed the issue before I submitted the PR but left it in because I felt it could be beneficial to others who might want to build the project but don't have much Jenkins, Travis, or Gradle experience.
The action only runs when triggered manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some unittests for es in
tests/src/test/scala/org/apache/openwhisk/core/database/elasticsearch/ElasticSearchActivationStoreTests.scala
which is usingtestcontainers
, so I think you can use it to run tests on your laptop?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It failed when trying to download one of the dependencies. I'll try again so I can get the dependency name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick update, I tried to build the project locally again and got the following error:
I must admit, this is my first time with Scala and Gradle, and while I think I might know what happened, I'm still trying to work out how to go about fixing it.