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

ISPN-13691 Feature form for persisted caches #219

Merged
merged 1 commit into from Jun 16, 2022

Conversation

dpanshug
Copy link
Collaborator

@dpanshug dpanshug commented Jun 7, 2022

Feature form for Persisted Cache

Copy link
Contributor

@ryanemerson ryanemerson left a comment

Choose a reason for hiding this comment

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

My review has focused on the templates. I haven't looked at the Typescript code.

IMO the templates used in the Wizard should provide the minimal configuration required for the store to function in it's most common use-case. I imagine a lot of users will attempt to utilise the templates without modification, so it's important that we don't include "extra" configuration such as "write-behind" that aren't applicable to the most common use-case.

src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
src/app/utils/persistentStorageTemplate.ts Show resolved Hide resolved
src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
src/services/infinispanRefData.ts Outdated Show resolved Hide resolved
@dpanshug
Copy link
Collaborator Author

dpanshug commented Jun 8, 2022

@ryanemerson new commit pushed. updated your suggestions, and now cache creation is also working in the last step.

@dpanshug dpanshug requested a review from ryanemerson June 9, 2022 05:58
Copy link
Contributor

@ryanemerson ryanemerson left a comment

Choose a reason for hiding this comment

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

Just the one outstanding comment to address and your branch needs rebasing as there are conflicts that prevent the PR from being mergeable.

src/app/utils/persistentStorageTemplate.ts Outdated Show resolved Hide resolved
@dpanshug dpanshug force-pushed the ISPN-13691-feature-persisted branch from c02974b to 17cce4b Compare June 14, 2022 10:55
@dpanshug
Copy link
Collaborator Author

@ryanemerson Branch rebased and the last suggestion was also done 👍

@ryanemerson ryanemerson self-requested a review June 14, 2022 11:21
@dpanshug
Copy link
Collaborator Author

@oraNod please review the labels :)

@dpanshug dpanshug force-pushed the ISPN-13691-feature-persisted branch from 4671297 to d1fc429 Compare June 15, 2022 05:50
@dpanshug
Copy link
Collaborator Author

@ryanemerson got this error while creating persistent cache with "rocks-db" storage
Screenshot from 2022-06-15 11-24-16

config:

{
  "distributed-cache": {
    "mode": "SYNC",
    "owners": 1,
    "statistics": true,
    "encoding": {
      "media-type": "application/x-protostream"
    },
    "locking": {
      "isolation": "REPEATABLE_READ"
    },
    "persistence": {
      "rocks-db": {
        "path": "rocksdb/data",
        "expiration": {
          "path": "rocksdb/expired"
        }
      }
    }
  }
}

@karesti
Copy link
Collaborator

karesti commented Jun 15, 2022

@dipguptaredhat the persistence stores are available only when the proper connectors are available in the server. I'm reviewing the PR from my side

@ryanemerson
Copy link
Contributor

got this error while creating persistent cache with "rocks-db" storage

I think the store parent element should be "rocksdb-store"

@karesti
Copy link
Collaborator

karesti commented Jun 15, 2022

@dipguptaredhat @ryanemerson @oraNod I'm on this PR

@karesti karesti force-pushed the ISPN-13691-feature-persisted branch 2 times, most recently from d76dd56 to e9c2970 Compare June 15, 2022 15:25
@karesti
Copy link
Collaborator

karesti commented Jun 15, 2022

@dipguptaredhat @oraNod I simplified after speaking with Ryan since having more than one store is VERY UNLIKE TO HAPEN. In general 1 cache = 1 store, so configuring multiple stores is confusing and complicated.
I also simplified the templates since the "table-store" and so on are part of the persistent store config and should not be in the end but in the editor as an example

@karesti karesti force-pushed the ISPN-13691-feature-persisted branch from e9c2970 to b4d5de8 Compare June 15, 2022 15:28
Copy link
Collaborator

@karesti karesti left a comment

Choose a reason for hiding this comment

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

needs some extra checks. for example if the config in the editor is json for persistence but the json is not valid, we should not be able to 'next'.

@karesti karesti force-pushed the ISPN-13691-feature-persisted branch from b4d5de8 to 9b4e797 Compare June 16, 2022 12:16
@karesti karesti force-pushed the ISPN-13691-feature-persisted branch from 9b4e797 to bfe705b Compare June 16, 2022 12:17
@karesti karesti merged commit 13abdd0 into infinispan:main Jun 16, 2022
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

3 participants