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

Issue 6734: LTS - Make CHUNKED_STORAGE default in ServiceConfig and InProcPravegaCluster. #6735

Merged

Conversation

sachin-j-joshi
Copy link
Contributor

@sachin-j-joshi sachin-j-joshi commented May 11, 2022

Signed-off-by: Sachin Joshi sachin.joshi@emc.com

Change log description
Issue 6734: LTS - Make CHUNKED_STORAGE default in ServiceConfig and InProcPravegaCluster.

Purpose of the change
Fixes #6734

What the code does

  • Change default values for storage to be CHUNKED_STORAGE
  • When reuseStorage is true then only the underlying InMemoryChunkStorage instance should be shared across containers not the SLTS instance.

How to verify it
All tests should pass.

…nProcPravegaCluster.

Signed-off-by: Sachin Joshi <sachin.joshi@emc.com>
@sachin-j-joshi
Copy link
Contributor Author

@AJadhav29 Please review.

@sachin-j-joshi sachin-j-joshi marked this pull request as draft May 11, 2022 23:10
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #6735 (ecd4190) into master (82f4393) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #6735   +/-   ##
=========================================
  Coverage     86.45%   86.45%           
  Complexity    15892    15892           
=========================================
  Files          1025     1025           
  Lines         59307    59306    -1     
  Branches       5988     5987    -1     
=========================================
+ Hits          51273    51275    +2     
+ Misses         4906     4904    -2     
+ Partials       3128     3127    -1     
Impacted Files Coverage Δ
...re/storage/mocks/InMemorySimpleStorageFactory.java 100.00% <100.00%> (ø)
...ga/controller/store/client/StoreClientFactory.java 81.81% <0.00%> (-2.28%) ⬇️
...ga/client/connection/impl/TcpClientConnection.java 89.04% <0.00%> (-2.06%) ⬇️
...egmentstore/storage/chunklayer/WriteOperation.java 94.19% <0.00%> (-1.34%) ⬇️
...a/segmentstore/server/logs/OperationProcessor.java 90.71% <0.00%> (-1.08%) ⬇️
...segmentstore/server/logs/DataFrameInputStream.java 78.57% <0.00%> (-0.80%) ⬇️
.../server/attributes/SegmentAttributeBTreeIndex.java 89.61% <0.00%> (-0.55%) ⬇️
.../segmentstore/server/tables/ContainerKeyIndex.java 93.54% <0.00%> (-0.30%) ⬇️
...ver/containers/StreamSegmentContainerMetadata.java 97.45% <0.00%> (+0.63%) ⬆️
...o/pravega/client/stream/impl/ReaderGroupState.java 93.49% <0.00%> (+0.90%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82f4393...ecd4190. Read the comment docs.

@RaulGracia
Copy link
Contributor

There are 2 tests failing with this change:

io.pravega.segmentstore.server.host.handler.AdminRequestProcessorImplTest.testUnsupportedOperation FAILED. Output (Last 300 lines):
 > 2022-05-12 16:54:51,737 370754 [Time-limited test] INFO  io.pravega.test.common.TestUtils - Available free port is 59639
 > 2022-05-12 16:54:51,745 370762 [Time-limited test] INFO  i.p.s.server.store.ServiceBuilder - Not attaching a SegmentContainerManager to ReadOnlyServiceBuilder.
 > 2022-05-12 16:54:51,752 370769 [Time-limited test] INFO  i.p.s.server.CacheManager - CacheManager Closed.

io.pravega.segmentstore.server.host.handler.AdminRequestProcessorImplTest > testUnsupportedOperation FAILED
    java.lang.UnsupportedOperationException: SimpleStorageFactory requires ChunkMetadataStore
        at io.pravega.segmentstore.storage.mocks.InMemorySimpleStorageFactory.createStorageAdapter(InMemorySimpleStorageFactory.java:58)
        at io.pravega.segmentstore.server.containers.ReadOnlySegmentContainer.<init>(ReadOnlySegmentContainer.java:87)
        at io.pravega.segmentstore.server.containers.ReadOnlySegmentContainerFactory.createStreamSegmentContainer(ReadOnly
      

io.pravega.segmentstore.server.host.handler.PravegaRequestProcessorTest.testUnsupportedOperation FAILED. Output (Last 300 lines):
 > 2022-05-12 16:52:05,192 200114 [Time-limited test] INFO  io.pravega.test.common.TestUtils - Available free port is 53139
 > 2022-05-12 16:52:05,197 200119 [Time-limited test] INFO  i.p.s.server.store.ServiceBuilder - Not attaching a SegmentContainerManager to ReadOnlyServiceBuilder.
 > 2022-05-12 16:52:05,200 200122 [Time-limited test] INFO  i.p.s.server.CacheManager - CacheManager Closed.

io.pravega.segmentstore.server.host.handler.PravegaRequestProcessorTest > testUnsupportedOperation FAILED
    java.lang.UnsupportedOperationException: SimpleStorageFactory requires ChunkMetadataStore
        at io.pravega.segmentstore.storage.mocks.InMemorySimpleStorageFactory.createStorageAdapter(InMemorySimpleStorageFactory.java:58)
        at io.pravega.segmentstore.server.containers.ReadOnlySegmentContainer.<init>(ReadOnlySegmentContainer.java:87)

@RaulGracia
Copy link
Contributor

@sachin-j-joshi the reason why these tests seem to fail is when combining starting a ReadOnlySegmentContainer with CHUNKED_STORAGE. I have tried to change that, adding a similar method to how we initialize storage in the regular StreamSegmentContainer:

However, I realized that ReadOnlySegmentContainer does not even contain metadata, and we cannot initialized ChunkedStorage without that. It makes sense because when ReadOnlySegmentContainer feature was created, there was only ROLLING_STORAGE, and the metadata for that is in storage as well. In other words, there was no Table Segments to write metadata to at that time. This also implies that ReadOnlySegmentContainer was not taken into account to adapt it to SLTS, and perhaps this PR could do so in order to make these failing tests to work (unless the amount of work to do that is too much).

@sachin-j-joshi
Copy link
Contributor Author

sachin-j-joshi commented Jun 2, 2022

Right. I'm going to split this PR into 2 parts.
Basically two different commits in this PR become two different PRs.

Signed-off-by: Sachin Joshi <sachin.joshi@emc.com>
Signed-off-by: Sachin Joshi <sachin.joshi@emc.com>
@sachin-j-joshi
Copy link
Contributor Author

@RaulGracia Given that we plan to eventually remove ReadOnlySegmentContainer , here it is better to just remove that problematic test that tests the operation that is not supported by ReadOnlySegmentContainer .

@sachin-j-joshi sachin-j-joshi marked this pull request as ready for review June 14, 2022 18:22
@sachin-j-joshi
Copy link
Contributor Author

@RaulGracia @abhinb This change is ready for review.

cc: @AJadhav29

Copy link
Contributor

@RaulGracia RaulGracia left a comment

Choose a reason for hiding this comment

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

Looks good, but there are conflicts to resolve in this PR.

…e-6734-make-slts-default

Signed-off-by: Sachin Joshi <sachin.joshi@emc.com>

# Conflicts:
#	segmentstore/server/host/src/test/java/io/pravega/segmentstore/server/host/handler/PravegaRequestProcessorTest.java
@sachin-j-joshi
Copy link
Contributor Author

Merged master. Conflict resolved.

@RaulGracia RaulGracia merged commit a10713f into pravega:master Jun 15, 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.

LTS: Make CHUNKED_STORAGE default in ServiceConfig and InProcPravegaCluster
2 participants