Skip to content

[Minor] Update documents in-line with doc standards#926

Merged
jerryshao merged 1 commit intoapache:mainfrom
justinmclean:doc-standards
Dec 6, 2023
Merged

[Minor] Update documents in-line with doc standards#926
jerryshao merged 1 commit intoapache:mainfrom
justinmclean:doc-standards

Conversation

@justinmclean
Copy link
Member

What changes were proposed in this pull request?

Update documents in-line with doc standards, mostly fixing title and passive voice.

Why are the changes needed?

For consitancy.

Fix: # N/A

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Built locally.

@github-actions
Copy link

github-actions bot commented Dec 4, 2023

Code Coverage Report

Overall Project 65.78% 🟢

There is no coverage information present for the Files changed

| `gravitino.entity.store.kv` | Detailed implementation of Kv storage, currently supported is `RocksDB` storage implementation `RocksDBKvBackend`. | `RocksDBKvBackend` | 0.1.0 |
| `gravitino.entity.store.kv.rocksdbPath` | Directory path of `RocksDBKvBackend`, **It's highly recommend that you change this default value** as it's under the deploy directory and future version upgrades may remove it. | `${GRAVITINO_HOME}/data/rocksdb` | 0.1.0 |
| `gravitino.entity.store.maxTransactionSkewTimeMs` | The maximum skew time of transactions in milliseconds. | `2000` | 0.3.0 |
| `gravitino.entity.store.kv.deleteAfterTimeMs` | The maximum time in milliseconds that the deleted data and old version data is kept. Set to at least 10 minutes and no longer than 30 days. | `604800000`(7 days) | 0.3.0 |
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have configuration doc in code, we should also change them to align to the doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want the document standard to apply to Java docs? That would be a large amount of work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so. We should align the configuration doc both in code and doc, otherwise it has differences as code/doc evolves.

Copy link
Member Author

Choose a reason for hiding this comment

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

IT would be better if we had a single source of truth and had the code refer to the docs or vice versa. If we have it in two places it is likely to get out of sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it will be out of sync, but currently we don't have better ways to be SSOT. Why not align them in this version, and we can find out better solution in the next version.

Copy link
Member Author

@justinmclean justinmclean Dec 5, 2023

Choose a reason for hiding this comment

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

Can you please point me to where this is in the code? Searching on gravitino.entity.store.kv etc etc fails to find any similar configuration mentioned in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@justinmclean the current configuration-related code spreads into many places, I can help you to modify the code part.

@justinmclean justinmclean self-assigned this Dec 5, 2023
@jerryshao jerryshao closed this Dec 6, 2023
@jerryshao jerryshao reopened this Dec 6, 2023
@jerryshao jerryshao merged commit f14684a into apache:main Dec 6, 2023
jerryshao added a commit that referenced this pull request Dec 6, 2023
…es (#989)

### What changes were proposed in this pull request?

This PR proposes to add code change to align with #926 doc change.

### Why are the changes needed?

Without this change, there will be a discrepancy between doc and code.

Fix: #979 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UTs.
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.

2 participants