-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
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.
Make sure to format the Java code in the same style as the rest of the project.
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetModule.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/PluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/PluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/PluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetModule.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/OfferSetModule.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/PluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/com/paypal/aurora/scheduler/offers/PluginConfig.java
Outdated
Show resolved
Hide resolved
Is this PR on the right repo? why is the package com.paypal.aurora? |
Hey @mauri, glad to see you're still around 😄 . We'll probably change the packaging name but we're upstreaming an offer sorting module that's able to use an external program with a defined interface to sort offers. The reason this is an HTTP interface is because we wanted to be able to use a program written in a different language to do this (we wrote ours in Go). We may maintain this sorter in a different repo but we figured we'd try to upstream it first. |
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.
Can we migrate the package from src/main/java/com/paypal/aurora...
to src/main/java/io/github/aurora-scheduler/...
?
Also we could use some documentation for the HTTP endpoints MagicMatch uses. That way someone else can come in and replace MagicMatch with something of their own.
In fact, we could rename this OfferSet module to be called something like HttpOfferSet
at this point. @zorro786 and @lenhattan86 any thoughts?
src/main/java/com/paypal/aurora/scheduler/offers/PluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpPluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpPluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpPluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
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.
Really good job @lenhattan86 this PR has come a long way from the initial version.
src/main/java/io/github/aurora/scheduler/offers/HttpPluginConfig.java
Outdated
Show resolved
Hide resolved
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.
Let's switch to using the Apache HTTP client and I think we're ready to ship!
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
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.
Great job @lenhattan86
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.
Once you fix these, LGTM.
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetModule.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/offers/HttpOfferSetImpl.java
Outdated
Show resolved
Hide resolved
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.
LGTM
Description:
The plugin sends the request and offer list to a plugin running on the same node with aurora.
It receives the response (sorted offers) from the plugin, and proceeds the task assignment process.
Testing Done: