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.

Sign up for GitHub

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

Jump to bottom

Use a fixed length segment descriptor #13240

Closed
deepthidevaki opened this issue Jun 29, 2023 · 3 comments
Closed

Use a fixed length segment descriptor #13240

deepthidevaki opened this issue Jun 29, 2023 · 3 comments
Assignees
@deepthidevaki
Labels
kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. version:8.3.0-alpha4 Marks an issue as being completely or in parts released in 8.3.0-alpha4 version:8.3.0 Marks an issue as being completely or in parts released in 8.3.0

Comments

@deepthidevaki
Copy link
Contributor

Follow up ##12839 (comment)

In PR #12839, when we added new fields to the segment descriptor the descriptor length changed. This can cause issues, when we overwrite an existing descriptor. We have to ensure that we don't accidently overwrite the first entry. This is handled in the PR. But, when ever we make changes to the descriptor this needs to be handled correctly. It is prone to errors, so it is better we use fixed size for the descriptor. We may not use the full allocated bytes, but it allows us to add new fields without the need to change the length again.

Besides, we also now have to keep track of length of each version. #12839 (comment) With fixed lenght, we might be able to handle this better.

Note: Better do it before 8.3, otherwise it will be more effort to make it backward compatible.
@deepthidevaki deepthidevaki added the kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. label Jun 29, 2023
@deepthidevaki deepthidevaki mentioned this issue Jun 29, 2023
Merged
14 tasks
@deepthidevaki
Copy link
Contributor Author

@npepinpe We discussed different things previously.

  1. As described in the issue, use a fixed length for descriptor. There will be unused bytes so that we can freely add or remove fields in future with out changing the descriptor length. This means we will have three versions - V1 (old one), V2 (version until 8.2 where the length = sbe encoded length), V3 with fixed length.
  2. We also discussed about removing the need to keep track of the version length because we can determine the length while decoding sbe. In this case we will have only two version - V1 (old one) and V2. In V2 depending on sbe version we may get different lengths. We can also add new fields in future which changes the length without changing the version because sbe version is updated.

I'm now thinking about which way to proceed. Any opinions?
I'm leaning towards option 2. The main problem with variable length is that a new version should not overwrite an existing descriptor with old version. But, this is already handled in the code. So I don't see any other blockers. With this, I don't see any advantage of using fixed length. What do you think?

@npepinpe
Copy link
Member

The second one as well 👍

@deepthidevaki deepthidevaki mentioned this issue Jul 26, 2023
Merged
14 tasks
@deepthidevaki
Copy link
Contributor Author

Closing this issue as we decided not to use fixed length descriptor. Instead PR #13618 improves the handling of version in segment descriptor.

@deepthidevaki deepthidevaki closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2023
ghost pushed a commit that referenced this issue Jul 31, 2023
@deepthidevaki @npepinpe
merge: #13618 #13706
d0d33bd 
13618: Remove unnecessary new version from segment descriptor r=deepthidevaki a=deepthidevaki

## Description

Remove keeping track of descriptor length for each version. Instead the length is determined while decoding sbe. As a consequence, we cannot check and fail early before the descriptor is read if the file size is not enough to store the descriptor. 

Since we rely on sbe to determine length, we can also remove version 3 which was introduced recently when new fields were added to the descriptor. 

In addition, this PR also refactors SegmentDescriptor and split the reading from buffer and decoding out of it.

Note:- This is backward **incompatible** with the previous SNAPSHOT and 8.3.0-alpha3, because we removed version 3. But it is still backward compatible with previous minor (8.2.x). 

## Related issues

related #13240 But we decided not use fixed length



13706: Optimize Dockerfile for CVE maintenance r=npepinpe a=npepinpe

## Description

Refactors our container image to reduce the effort related to CVE maintenance by:

- Build the final target on top of the latest available LTS Ubuntu
  - This lets us track Ubuntu updates much faster than via the Temurin image
- Upgrade all install required packages to the latest on build
- Build a custom JRE out of the official Temurin JDK
  - This lets us keep up to date with Temurin updates
  - As a bonus, we get to have a smaller JRE (via module compression), and keep useful production utilities: `jcmd`, `jmap`, `jps`
- The new image has a lower vulnerability count:
  - 12 issues across 203 dependencies: 3 med, 9 low
  - versus 20 issues across 237 dependencies: 4 med, 16 low
- The new image is smaller:
  - 357 MB over 8 layers: base (78), packages (3.8), JDK (101), and Zeebe (174)
  - 444 MB over 9 layers: base (73), packages (57), JDK (141), and Zeebe (174)

The main difference between using our own Java image versus the Temurin base is the smaller JRE (due to module compression), and two packages I haven't installed: locales, and tzdata. These contain language packs and time zone info, two features we don't really use in Zeebe.

> **Note**
> This could affect third party code such as exporters and interceptors, so we may want to reintroduce this to provide an as-close-as-possible experience to stock JDK for users. However, I don't know if we have any real data to validate this. We don't do i18n (and I don't expect exporters or interceptors to as well), and it's better to deal with UTC in your producing system and only change it in your clients anyway.

> **Note**
> The custom JRE idea comes from the Identity team, so kudos for that 🎉 

## Related issues

closes #13214 
closes #12959



Co-authored-by: Deepthi Devaki Akkoorath <[email protected]>
Co-authored-by: Nicolas Pepin-Perreault <[email protected]>
@megglos megglos added the version:8.3.0-alpha4 Marks an issue as being completely or in parts released in 8.3.0-alpha4 label Aug 2, 2023
@megglos megglos added the version:8.3.0 Marks an issue as being completely or in parts released in 8.3.0 label Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deepthidevaki deepthidevaki

Labels
kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. version:8.3.0-alpha4 Marks an issue as being completely or in parts released in 8.3.0-alpha4 version:8.3.0 Marks an issue as being completely or in parts released in 8.3.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@npepinpe @megglos @deepthidevaki