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.

Sign up for GitHub

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

Jump to bottom

Document that the default TTL is 60 seconds #37

Open
crccheck opened this issue Jan 24, 2017 · 1 comment
Open

Document that the default TTL is 60 seconds #37

crccheck opened this issue Jan 24, 2017 · 1 comment

Comments

@crccheck
Copy link

The README currently doesn't say this
stripethree pushed a commit to CondeNast/launch-vehicle-fbm that referenced this issue Feb 14, 2017
@crccheck
Bump cacheman TTL from 60 seconds to 1 hour (#210)
6b67485 
## Why are we doing this?

While investigating losing selfies #193 , it doth appear that [Cacheman's default TTL is 60 seconds](https://github.com/cayasso/cacheman/blob/6149f436268cedbec54633c81481ae7035411873/lib/index.js#L99-L104) but [it's not documented](cayasso/cacheman#37). So it was dropping data. This raises it to match our definition of a user session, one hour.

## Did you document your work?

This explicitly sets the TTL, which increases documentations

## How can someone test these changes?

Steps to manually verify the change:

1. Using the `master` branch
1. I started a bot
2. talked to the bot, noting the `count` log
```
lenses:messenger message.text user:1250872178269050 text: "fart" count: 1 seq: 8760
```
3. wait over a minute and talk again, the `count` will go back to `1`.
5. do the same thing with this branch, but the `count` will keep going up.

## What possible risks or adverse effects are there?

I haven't looked into cacheman's internals, but this is effectively the same as a memory leak. We can transition to another backend if this is a problem. As it is, we're not using a lot of RAM.

## What are the follow-up tasks?

* watch #193 

## Are there any known issues?

none

## Did the test coverage decrease?

same, initial values to cacheman are not part of the test suite.
@crccheck crccheck mentioned this issue Mar 13, 2017
Merged
crccheck added a commit to CondeNast/launch-vehicle-fbm that referenced this issue Mar 13, 2017
@crccheck
Fix session timeouts (#40)
203c09c 
## Why are we doing this?

This is a copy paste of a fix that wasn't copy pasted into the initial SDK. Cacheman has a default TTL of [60 seconds](https://github.com/cayasso/cacheman/blob/6149f436268cedbec54633c81481ae7035411873/lib/index.js#L99-L104) but [it's not documented](cayasso/cacheman#37). This meant if you took a break in the middle of a quiz, it would freeze on you.

## Did you document your work?

This explicitly sets the TTL, which increases documentation

## How can someone test these changes?

Can't test within the SDK, but this is a copy paste

## What possible risks or adverse effects are there?

* Uptime
* Security
* Performance degradation
* Data loss

If applicable, include a technical debt note. Do a quick estimate of if this pull request increases or decreases technical debt. Leave a message to future developers about why increased technical debt is worth it, or brag about how you figured out how to reduce technical debt.

## What are the follow-up tasks?

* Make setting the session length a constructor option to the SDK

## Are there any known issues?

None

## Did the test coverage decrease?

same, initial values to cacheman are not part of the test suite.
@DylanPiercey
Copy link

IMO it should default to Number.MAX_VALUE. 😄

@roy7 roy7 mentioned this issue Jan 4, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@crccheck @DylanPiercey