Lwcapi merge #459
Lwcapi merge #459
Conversation
This reverts commit 6b519bd.
| atlas { | ||
| lwcapi { | ||
| register = { | ||
| default-frequency = 60000 |
There was a problem hiding this comment.
Use duration format so it is easier to read and the unit is explicit.
| redis = { | ||
| host = "localhost" | ||
| port = 6379 | ||
| ttl = 300000 // in milliseconds, default 5 minutes |
There was a problem hiding this comment.
See above comment on duration format.
|
|
||
| def redisHost: String = config.getString("redis.host") | ||
| def redisPort: Int = config.getInt("redis.port") | ||
| def redisTTL: Int = config.getInt("redis.ttl") |
There was a problem hiding this comment.
| override def isHealthy: Boolean = state == State.RUNNING && started | ||
|
|
||
| override def startImpl(): Unit = { | ||
| logger.info("Starting Database service monitor") |
There was a problem hiding this comment.
The lifecycle manager will already have logs for calling the start/stop, e.g.:
| class DatabaseService extends AbstractService with StrictLogging { | ||
| @volatile private var started = false | ||
|
|
||
| def setState(value: Boolean): Unit = started = value |
There was a problem hiding this comment.
Locally to this class, it seems odd to have setState modify the started variable when there is another state variable in scope.
There was a problem hiding this comment.
I thought "state" was the state of the registration, while "started" really means "initial transfer from other instances via redis is complete"
I can rename things to to make it clearer, and add commends, unless I really can / should touch "state" here.
| registry.counter(messagesId.withTag("action", msg.getWhat).withTag("streamId", sseId)).increment() | ||
| } else { | ||
| droppedCount += 1 | ||
| registry.counter(droppedId.withTag("action", msg.getWhat).withTag("streamId", sseId)).increment() |
There was a problem hiding this comment.
It would be a little more efficient to do: withTags("action", msg.getWhat, "streamId", sseId).
| } | ||
| } | ||
|
|
||
| private def handleReq(ctx: RequestContext, streamId: String, name: Option[String], expr: Option[String], freqString: Option[String]): Unit = { |
There was a problem hiding this comment.
Try to wrap at 100 chars.
| path("lwc" / "api" / "v1" / "stream" / Segment) { (streamId) => | ||
| parameters('name.?, 'expression.?, 'frequency.?) { (name, expr, frequency) => | ||
| get { (ctx) => handleReq(ctx, streamId, name, expr, frequency) } ~ | ||
| post { (ctx) => handleReq(ctx, streamId, name, expr, frequency) } |
There was a problem hiding this comment.
A post that takes query params on the URL seems a bit odd.
There was a problem hiding this comment.
I could make it so that the post will not accept params, but the get will, now that the post body is used. This was originally just to make get and post be the same, since apparently some firewalls have issues with SSE over get requests, but not posts.
| } else if (-item != next.get._1) { | ||
| // requeue with the latest timestamp | ||
| prio.dequeue | ||
| prio += ((-item, key)) |
| def touch(key: K, now: Long): Unit = { | ||
| val old = touched.put(key, now) | ||
| if (old.isEmpty) | ||
| prio += ((-now, key)) |
There was a problem hiding this comment.
Add comment about why this is negated.
Still needs a lot of work, but is a minimal base version that will send data at the same frequency as publishing to Atlas. For more details on the server side see: Netflix/atlas#459. Future iteration needed for refactoring to honor frequency in the subscription, performance tuning, etc.
Still needs a lot of work, but is a minimal base version that will send data at the same frequency as publishing to Atlas. For more details on the server side see: Netflix/atlas#459. Future iteration needed for refactoring to honor frequency in the subscription, performance tuning, etc.
Still needs a lot of work, but is a minimal base version that will send data at the same frequency as publishing to Atlas. For more details on the server side see: Netflix/atlas#459. Future iteration needed for refactoring to honor frequency in the subscription, performance tuning, etc.
Merge in lwcapi. This adds the requirement for travis to run a redid-server, although that is intended to go away in the future.