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

chore(sdk) [NET-1394]: add new storage node cluster address #3020

Merged
merged 7 commits into from
Feb 25, 2025

Conversation

harbu
Copy link
Contributor

@harbu harbu commented Feb 20, 2025

Summary

  • Add new storage node cluster address constant STREAMR_STORAGE_NODE_ADDRESS
  • Deprecate existing constant STREAMR_STORAGE_NODE_GERMANY
  • Adjust docs to reflect this change

Checklist before requesting a review

  • Is this a breaking change? If it is, be clear in summary.
  • Read through code myself one more time.
  • Make sure any and all TODO comments left behind are meant to be left in.
  • Has reasonable passing test coverage?
  • Updated changelog if applicable.
  • Updated documentation if applicable.

Copy link

linear bot commented Feb 20, 2025

@github-actions github-actions bot added the sdk label Feb 20, 2025
@github-actions github-actions bot added the docs label Feb 20, 2025
@harbu harbu marked this pull request as ready for review February 20, 2025 15:40
@harbu harbu requested review from mondoreale and teogeb February 20, 2025 16:07
export const STREAMR_STORAGE_NODE_GERMANY = '0x31546eEA76F2B2b3C5cC06B1c93601dc35c9D916'

export const STREAMR_STORAGE_DEFAULT = '0x9dc08ff97f5c156181ec6a0b13fc3946454e529a'
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to be very explicit, this could be named to STREAMR_STORAGE_NODE_ADDRESS and/or type this as HexString. Then it would be trivial that this is a value that can be given as an argument to addStreamToStorageNode().

Copy link
Contributor Author

@harbu harbu Feb 20, 2025

Choose a reason for hiding this comment

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

I'll add the HexString type.

About the naming, it is a bit complicated. When we refer to a storage node, we don't actually refer to a single storage node (most of the time) but a cluster of storage nodes. The correct name would therefore be something like STREAMR_STORAGE_NODE_CLUSTER_DEFAULT. However, our internal naming convention currently refers to the address as storageNodeAddress so keeping in line with that it may be wise to stick with the NODE for now. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

As you mentioned, end-users don't know about the clustering. In SDK's nomenclature the entity to which we add/remove streams in order to get it stored is a storage node. This is an id to such entity, and therefore we should refer it as storage node.

In practice the entity can be a single node or a cluster of nodes. As long as a real cluster (i.e more than one node) is not required, the current name is quite fine.

In summary: in my opinion we should now keep the current consistent term (STREAMR_STORAGE_NODE_ADDRESS). We can consider renaming the terminology in a follow-up PR, if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed here 6a651e8

@harbu harbu requested a review from teogeb February 24, 2025 11:10
Copy link
Contributor

@teogeb teogeb left a comment

Choose a reason for hiding this comment

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

Maybe would make sense to drop the term "cluster" also in CHANGELOG (lines 15 and 21)

@harbu harbu merged commit 88b8141 into main Feb 25, 2025
24 checks passed
@harbu harbu deleted the NET-1394-add-new-storage-cluster-address-to-sdk branch February 25, 2025 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants