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

Remove AS uplink storage #6317

Merged
merged 25 commits into from
Jul 4, 2023
Merged

Conversation

cvetkovski98
Copy link
Member

@cvetkovski98 cvetkovski98 commented Jun 8, 2023

Summary

The AS Uplink Storage is used only for the LoRa Geolocation Services integration. This PR moves its functionality to the GLS association data, adds appropriate methods to the association registry for cleaning association when a device or application link is removed and an appropriate migration for the removed functionality.

This PR also removes the automatic determination of the window size based on the frame payload of the GLS integration and limits the frame size in the [1, 16] range. This can be set through the Console.

Closes #6215

Changes

  • Deprecate and remove the uplink storage registry
  • Refactor the multi-frame queries in the GLS integration to use the association data
  • Add Clear methods to the application registry store.

Testing

  • UT
  • Manual
Regressions

...

Notes for Reviewers

...

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@cvetkovski98 cvetkovski98 linked an issue Jun 8, 2023 that may be closed by this pull request
12 tasks
@cvetkovski98 cvetkovski98 changed the title Feature/remove as uplink storage Remove AS uplink storage Jun 8, 2023
@github-actions github-actions bot added c/application server This is related to the Application Server compat/config This could affect Configuration compatibility compat/db This could affect Database compatibility labels Jun 8, 2023
@adriansmares adriansmares added this to the v3.26.2 milestone Jun 9, 2023
Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

This is going in the right direction and looks correct so far. My only change to the design is to optimize a bit storage, as saving the field names has no benefit since only the AS has to be able to encode and decode the data.

cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
pkg/applicationserver/applicationserver.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/data.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/data.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/data.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/redis/registry.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/redis/registry.go Outdated Show resolved Hide resolved
@cvetkovski98 cvetkovski98 force-pushed the feature/remove-as-uplink-storage branch 6 times, most recently from 1158ea8 to faf1a09 Compare June 15, 2023 14:04
@cvetkovski98 cvetkovski98 marked this pull request as ready for review June 15, 2023 14:14
cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
pkg/applicationserver/applicationserver.go Outdated Show resolved Hide resolved
pkg/applicationserver/config.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/package.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/package.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/structs.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/structs.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/structs.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/redis/registry.go Outdated Show resolved Hide resolved
@cvetkovski98 cvetkovski98 force-pushed the feature/remove-as-uplink-storage branch 2 times, most recently from f2dd469 to 7af55c9 Compare June 16, 2023 09:58
@cvetkovski98 cvetkovski98 force-pushed the feature/remove-as-uplink-storage branch 2 times, most recently from 3c2aefb to d8e504b Compare June 16, 2023 10:36
Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

We should add a changelog entry for these changes. I would say that the migration is the main point to put an emphasis on, and the deprecated configuration options.

pkg/applicationserver/config.go Outdated Show resolved Hide resolved
pkg/applicationserver/io/packages/loragls/v3/package.go Outdated Show resolved Hide resolved
cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

LGTM. @johanstokking could you also take a look ?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

So this is only unit tested; are you unable to do a local integration test by purging data as well as seeing if GLS still works?

cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
pkg/applicationserver/applicationserver.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cmd/ttn-lw-stack/commands/as_db.go Outdated Show resolved Hide resolved
@cvetkovski98 cvetkovski98 force-pushed the feature/remove-as-uplink-storage branch from 3311ac4 to bc87da8 Compare June 26, 2023 10:14
@cvetkovski98 cvetkovski98 force-pushed the feature/remove-as-uplink-storage branch from a44d2fd to 57e9872 Compare July 3, 2023 11:50
@cvetkovski98 cvetkovski98 merged commit 2acf07e into v3.27 Jul 4, 2023
@cvetkovski98 cvetkovski98 deleted the feature/remove-as-uplink-storage branch July 4, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/application server This is related to the Application Server compat/config This could affect Configuration compatibility compat/db This could affect Database compatibility ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Application Server uplink storage
5 participants