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

Fix data stream batching #32

Merged
merged 1 commit into from
May 2, 2024
Merged

Fix data stream batching #32

merged 1 commit into from
May 2, 2024

Conversation

davidscavnicky
Copy link
Contributor

@davidscavnicky davidscavnicky commented Apr 13, 2024

data was gathered in cyclic manner MutableDataStreamBatch, inserted class into appendToDataStreams() so upon next appendToDataStream the class is initialized again.

The issue and discussion:
#16

@xelahalo
Copy link
Collaborator

This is sufficient for a quick workaround (given that it works), but the real underlying issue with the entire data streams implementation is that it was blindly copied over from core and wasn't given much thought.

The real question you have to ask here is: what does MutableDataStreamBatch do in our code base in the first place?
Imo that implementation is just a crutch in Core for the in memory data streams implementation, nothing else. There, the property's purpose was to store the data.

In CAWS, we don't need this representation, as we can just use the batch as is, and dump the sequences in the db (given that its conditions are met).

Copy link
Contributor Author

@davidscavnicky davidscavnicky left a comment

Choose a reason for hiding this comment

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

  • fixed accumulation of data
  • compared code base with Core package ( duplicates of code )
  • tested with Core and created test for DataStreamCoreService

Copy link
Contributor Author

@davidscavnicky davidscavnicky left a comment

Choose a reason for hiding this comment

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

changes based on comments, + generics + quick fix RecruitmentServiceTest, so all test run successfully

Copy link
Contributor Author

@davidscavnicky davidscavnicky left a comment

Choose a reason for hiding this comment

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

fixed accumulation of data
compared code base with Core package ( duplicates of code )
tested with Core and created test for DataStreamCoreService

Copy link
Contributor Author

@davidscavnicky davidscavnicky left a comment

Choose a reason for hiding this comment

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

all discussed fixed, + RecruitmentServiceTest fix constructor, but not the test run

@xelahalo xelahalo changed the title Inconsistent data on phone versus server Fix data stream batching May 2, 2024
@xelahalo xelahalo merged commit 037006d into develop May 2, 2024
xelahalo added a commit that referenced this pull request May 7, 2024
* Check if claims exist before account parsing

* Apply runBlocking on repository level

We apply runBlocking again, as it turns out, that our JDBC connector cannot
handle suspend functions,and is always doing blocking calls. We will need
to change the persistence layer if we want to deal with this properly.

* Add remove option to deployment script

* Add participant as a default role in keycloak

* Keycloakify: theme info page and edit Privacy Policy text (#14)

* Fix some text

* Make info page use Carp theme

* Upgrade to kotlin 1.9.23 and related dependencies

* Upgrade CARP Core to 1.2.0

* Adjust default token lifespans

* Clean up environment urls

* Remove caffeine caching

* Add controller level UUID conversion

* Prepare account services for anonymous participants

* Generate anonymous participants

* Fix docker compose CAWS port exposing

* Toggle off email as username

* Give unique name to anonymous accounts csv

* Redirect to parameter instead of portal for anonymous accounts

As a workaround we make magic-links redirect to the study instead of the portal, as the portal is not in an adequate state to be able to parse and do the redirects. This will have to be reverted (and given more thought) after the ICAT study is done and there is a need for anonymous deployments for generic web/mobile-hosted studies.

* Disable statistics endpoint

* Rewrite authorization to use OAuth claims

* Authorize ApplicationServices

* Readd logging

* Remove researcher reference from study

* Bump versions

- gradle from 7.4.2 to 8.7
- spring boot from 3.1.0 to 3.2.4
- spring addons from 6.1.2 to 7.6.12
- hibernate from 6.2.7 to 6.4.4

* Extend keycloak config to include claims for non-core endpoints

* Use IO dispatcher for repository calls

* Upgrade Keycloak theme (#42)

* Update keycloakify to v9

* Migrate existing pages

* Remove duplicate protocol mapper

* Update theme name

* Built new jar file

* Fix Dockerfile gradle version

* Add missing client scope mapper to default realm

* Fix typo in application properties

* Keycloak theme: fix broken button text on info page (#43)

* Change required claims for consent documents (#45)

* Make security expressions accept nullable values

* Make every claim String-based

The default Spring Addons authority converters can't deal with non-string values.

* Stop granting ManageDeployment Claims to Researchers

* Get authentication from the original thread when changing claims from non-suspend functions

* Make study owners be able to get all consent documents again

* Improve button style for Input page and overall responsiveness (#44)

* Improve button styles for info page

* Build updated .jar file for KC theme

* Revoke claims through authorizers when resource gets deleted (#46)

This solution will not revoke the claims if the resources get deleted through a call from the services event bus. However that shouldn't be a big issue, if one doesn't use the claims in a way that presumes the existence of the referenced objects.

* Parallelize anonymous account request (#49)

* Add parallelization for anonymous accounts

* Optimize study overview

* Add getting protocols overview (#22)

Co-authored-by: xelahalo <olahalex8@gmail.com>

* Add created by to study overview (#50)

* Fix data stream batching (#32)

Co-authored-by: xelahalo <olahalex8@gmail.com>

* Use MS Teams for notifications (#54)

* add basic notification for MS-teams

* add notification channels corrsponding to MS teams channel, renamed/removed notificattions to Slack channels

* add teams channel notification for development server/profile

* Update .env

* rename methods

* update notification related comments

* Release 1.2.0

---------

Co-authored-by: Aamir Farooq <SlimShadyIAm@users.noreply.github.com>
Co-authored-by: David Scavnicky <109144925+davidscavnicky@users.noreply.github.com>
Co-authored-by: Yuan Chen <10835085+yuanchen233@users.noreply.github.com>
@xelahalo xelahalo deleted the feature/data-mismatch branch May 7, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants