Skip to content

Conversation

@dkropachev
Copy link

@dkropachev dkropachev commented Apr 7, 2025

Currently tabletMap is initialized once at DefaultMetadata.EMPTY As result all the session instances have same tabletMap, which can lead to all sort of problems.
This commit postpone tabletMap initialization and changes Metadata API to reflect these changes.

Partially addresses: #504

@dkropachev dkropachev requested a review from Bouncheck April 7, 2025 11:10
@dkropachev dkropachev self-assigned this Apr 7, 2025
@dkropachev dkropachev force-pushed the dk/4.x-postpone-tabletmap-initialization branch 2 times, most recently from 93570fa to ed0eab6 Compare April 7, 2025 11:22
Currently `tabletMap` is initialized once at `DefaultMetadata.EMPTY`
As result all the session instances have same `tabletMap`, which can lead
to all sort of problems.
This commit postpone tabletMap initialization and changes Metadata API
to reflect these changes.
@dkropachev dkropachev force-pushed the dk/4.x-postpone-tabletmap-initialization branch from ed0eab6 to 614f288 Compare April 7, 2025 11:22
Copy link

@Bouncheck Bouncheck left a comment

Choose a reason for hiding this comment

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

While this probably works would it not be better to eventually decouple TabletMap from TokenMap and have it work fully separately? Binding tablet map initialization to token map will require in the future that token map is always running too.

Alternatively this can be fixed with just changing

  public static DefaultMetadata EMPTY =
      new DefaultMetadata(
          Collections.emptyMap(), Collections.emptyMap(), null, null, DefaultTabletMap.emptyMap());

into

  public static DefaultMetadata empty() {
      return new DefaultMetadata(
          Collections.emptyMap(), Collections.emptyMap(), null, null, DefaultTabletMap.emptyMap());
}

@Bouncheck
Copy link

Bouncheck commented Apr 7, 2025

Ah, its not part of rebuildTokenMap but withNodes. Then i guess its fine.

@Bouncheck Bouncheck self-requested a review April 7, 2025 11:42
@dkropachev
Copy link
Author

While this probably works would it not be better to eventually decouple TabletMap from TokenMap and have it work fully separately? Binding tablet map initialization to token map will require in the future that token map is always running too.

Alternatively this can be fixed with just changing

  public static DefaultMetadata EMPTY =
      new DefaultMetadata(
          Collections.emptyMap(), Collections.emptyMap(), null, null, DefaultTabletMap.emptyMap());

into

  public static DefaultMetadata empty() {
      return new DefaultMetadata(
          Collections.emptyMap(), Collections.emptyMap(), null, null, DefaultTabletMap.emptyMap());
}

They are currently couple via Partitioner, which will be addressed at #502

@dkropachev
Copy link
Author

All test failures not related to the PR.

@dkropachev dkropachev merged commit 4c9d702 into scylladb:scylla-4.x Apr 7, 2025
6 of 10 checks passed
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