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

KV and OS Compression and Metadata #1034

Merged
merged 14 commits into from
Nov 8, 2023
Merged

KV and OS Compression and Metadata #1034

merged 14 commits into from
Nov 8, 2023

Conversation

scottf
Copy link
Contributor

@scottf scottf commented Oct 30, 2023

  • Add compression and metadata support to KV and OS
  • Some test efficiency improvements.
  • JsonValue utility improvements to support toString for KV and OS objects
  • Removal of EXPERIMENTAL label on Object Store

Also less generic test names for efficiency
*/
public long getMaxBucketSize() {
return sc.getMaxBytes();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved here because it's common to KV and OS

return sc.getCompressionOption() == JS_COMPRESSION_YES;
}

protected static abstract class Builder<B, FC> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved here b/c common to KV and OS. Should have been this way originally.

*/
public long getMaxBucketSize() {
return sc.getMaxBytes();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

common to KV and OS

*/
public Republish getRepublish() {
return sc.getRepublish();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

common to KV and OS

@@ -346,7 +346,7 @@ public KeyValueConfiguration build() {
.build());
}
}
else if (sources.size() > 0) {
else if (!sources.isEmpty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intellij hounds me about these. isEmpty checks the internal size variable. not sure why it always wants this, but it's the same amount of function calls

public long getMaxBucketSize() {
return sc.getMaxBytes();
}

Copy link
Contributor Author

@scottf scottf Nov 2, 2023

Choose a reason for hiding this comment

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

common to KV and OS

@scottf scottf changed the title KV and OS Compression KV and OS Compression and Metadata Nov 3, 2023
@@ -668,7 +668,6 @@ enum Status {

/**
* Gets a context for working with an Object Store.
* OBJECT STORE IMPLEMENTATION IS EXPERIMENTAL AND SUBJECT TO CHANGE.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing compatibility tests, can remove the EXPERIMENTAL label.

mb.put("metaData", getMetadata());
return mb.toJsonValue();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this just to have easy toString()

*/
public Mirror getMirror() {
return sc.getMirror();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added missing getter

*/
public List<Source> getSources() {
return sc.getSources();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added missing getter

@@ -157,8 +170,7 @@ public Builder(KeyValueConfiguration kvc) {
* @return the builder
*/
public Builder name(String name) {
this.name = validateBucketName(name, true);
return this;
return super.name(name);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not have to override these, but it allows me to have more specific api comments.

@@ -442,7 +442,7 @@ public boolean isDiscardNewPerSubject() {
}

/**
* Metadata for the consumer
* Metadata for the stream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad old copy paste fixed.

@@ -49,6 +49,8 @@ public enum Type {
public final Object object;
public final Number number;

public final List<String> mapOrder;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

helps toString to run through the map keys in a specific order, instead of whatever the keySet iterator would give.

@@ -347,6 +347,10 @@ public static String variant(Object variant) {
return variant == null ? NUID.nextGlobalSequence() : "" + variant;
}

public static String variant() {
return NUID.nextGlobalSequence();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes in this class are part of making the tests more efficient

Copy link
Contributor

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

LGTM

@scottf scottf merged commit 4ab3e28 into main Nov 8, 2023
2 checks passed
@scottf scottf deleted the kv-os-compression-and-aroc branch November 8, 2023 21: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