[WIP] LogBuffer metadata store channelUri #1697

pveentjer · 2024-12-12T08:42:00Z

In this PR the channelUri is added to the metadata section of the LogBuffer.

aeron-client/src/main/java/io/aeron/ChannelUriStringBuilder.java

vyazelenko · 2024-12-12T08:50:10Z

aeron-client/src/main/java/io/aeron/logbuffer/LogBufferDescriptor.java

+    /**
+     * The maximum byte of the channel-uri stores as an ascii string (excluding the length).
+     */
+    public static final int LOG_CHANNEL_URI_MAX_LENGTH = 4096 ;


There will be ChannelUri.MAX_URI_LENGTH that is set to 4095, i.e. we don't need log buffer specific limit.

4095 or 4096? I guess the latter.

I guess for /0 termination of C based strings.

vyazelenko · 2024-12-12T08:50:16Z

aeron-client/src/main/java/io/aeron/logbuffer/LogBufferDescriptor.java

+
+        LOG_META_DATA_LENGTH = align(LOG_CHANNEL_URI_OFFSET + SIZE_OF_INT + LOG_CHANNEL_URI_MAX_LENGTH, PAGE_MIN_SIZE);
+
+        System.out.println("offset:"+offset);


All the system.outs will removed in the final version of this PR.

vyazelenko · 2024-12-12T08:55:24Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

@@ -1764,15 +1819,66 @@ private NetworkPublication newNetworkPublication(
        }
    }

+
+    private static String enrichNetworkPublicationUri(ChannelUri channelUri, int streamId, int sessionId, int initialTermId, SendChannelEndpoint channelEndpoint, PublicationParams params)


You could create a base enrich method that sets common parameters such as mtu, term-length, initial-term-id (etc.) and will return a ChannelUriStringBuilder so that it can be further enhanced.

vyazelenko · 2024-12-12T08:56:34Z

aeron-driver/src/main/java/io/aeron/driver/Main.java

+
+import java.nio.ByteBuffer;
+
+public class Main


Is this a debug code? If not, then it should be turned into a a test instead.

It will also be removed.

vyazelenko · 2024-12-12T09:00:29Z

aeron-client/src/main/java/io/aeron/ChannelUriStringBuilder.java

+    public int streamId(){
+        return streamId;
+    }
+
+    public void streamId(int streamId)
+    {
+        this.streamId = streamId;
+    }
+


These should return and accept and Integer object. Also you need a version that reads stream-id from a ChannelUri (e.g. see ChannelUriStringBuilder#termOffset(io.aeron.ChannelUri)). And it needs to filled in the constructor of the class.

I have fixed the Integer part. I forget to change the signatures of the methods after converting streamId from int to Integer.

But the streamId isn't part of the channelUri; you can't create a channel like this:

Publication publication = aeron.addPublication( "aeron:udp?endpoint=localhost:40123|streamId=10");

Exposing the property as PARAM_NAME and adding it to the ChannelURI, could lead to end users getting confused. WDYT.

We want to make this parameter official. It should be possible to set stream-id in the channel uri and will be adding validation that the stream-id parameter is the same as streamId argument to the method.

vyazelenko · 2024-12-12T10:31:52Z

aeron-client/src/main/java/io/aeron/logbuffer/LogBufferDescriptor.java

+     * @throws IllegalArgumentException if the URI exceeds the maximum length.
+     */
+    public static void channelUri(final UnsafeBuffer metadataBuffer, final String channelUri) {
+        if (channelUri.length() > LOG_CHANNEL_URI_MAX_LENGTH) {


Should use ChannelUri.MAX_URI_LENGTH instead.

I introduced a new constant:

LOG_CHANNEL_URI_MAX_LENGTH = LOG_META_DATA_LENGTH - (LOG_CHANNEL_URI_OFFSET + SIZE_OF_INT);

This is now used to validate that the stored channel-uri isn't too long. This value is now 7164. Perhaps I should remove 1 more to deal with /0 C strings.

The extra length is needed because we'll be adding settings to the user specified channel-uri, so we could exceed the current 4095 limit.

vyazelenko · 2024-12-16T08:49:12Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+        final String ccStr = udpChannel.channelUri().get(CONGESTION_CONTROL_PARAM_NAME);
+        uriStringBuilder.congestionControl(null == ccStr ? "static" : ccStr);


The cc part can be removed as it is configured on a subscription side.

vyazelenko · 2024-12-16T08:50:31Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+        uriStringBuilder.initialTermId(params.initialTermId);
+        uriStringBuilder.termId(params.hasPosition ? params.termId : params.initialTermId);
+        uriStringBuilder.termOffset(params.hasPosition ? params.termOffset : 0);


This logic is already implemented in PublicationParams, i.e. just assign variables directly.

vyazelenko · 2024-12-16T08:51:24Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+
+        uriStringBuilder.eos(params.signalEos);
+        uriStringBuilder.spiesSimulateConnection(params.spiesSimulateConnection);
+    }


pub-wnd is missing, i.e. use params.publicationWindowLength to assign it.

vyazelenko · 2024-12-16T08:52:07Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+        if(!uriStringBuilder.isSessionIdTagged()){
+            uriStringBuilder.sessionId(params.sessionId);
+        }


I think we should write session-id unconditionally.

vyazelenko · 2024-12-16T08:52:22Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+        System.out.println("Set channelUri:" + channelUri(logMetaData));
    }


vyazelenko · 2024-12-16T08:52:49Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+        final PublicationParams params)
+    {
+        final ChannelUriStringBuilder uriStringBuilder = new ChannelUriStringBuilder(channel);
+        uriStringBuilder.streamId(streamId);


streamId is part of PublicationParams.

vyazelenko · 2024-12-16T08:53:04Z

aeron-client/src/main/java/io/aeron/logbuffer/LogBufferDescriptor.java

+        System.out.println("LOG_DEFAULT_FRAME_HEADER_MAX_LENGTH:" + LOG_DEFAULT_FRAME_HEADER_MAX_LENGTH);
+        System.out.println("LOG_CHANNEL_URI_OFFSET:" + LOG_CHANNEL_URI_OFFSET);
+        System.out.println("LOG_META_DATA_LENGTH:" + LOG_META_DATA_LENGTH);
+        System.out.println("LOG_CHANNEL_URI_MAX_LENGTH:" + LOG_CHANNEL_URI_MAX_LENGTH);


vyazelenko · 2024-12-16T08:53:55Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+        configureUriStringBuilderWithPublicationParams(params, uriStringBuilder);
+
+
+        uriStringBuilder.streamId(streamId);


streamId is part of PublicationParams.

aeron-client/src/main/java/io/aeron/ChannelUriStringBuilder.java

vyazelenko · 2024-12-17T12:22:44Z

aeron-client/src/main/java/io/aeron/ChannelUriStringBuilder.java

+    private Integer streamId;
+    private Integer publicationWindowLength;


This is already implemented on master. Please merge those into your PR or rebase it.

vyazelenko · 2024-12-17T12:27:21Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+
+        channelUriStringBuilder.receiverWindowLength(params.receiverWindowLength);
+
+      //  channelUriStringBuilder.group(params.group);


We should add so-rcvbuf and so-sndbuf from ReceiveChannelEndpoint.

vyazelenko · 2024-12-17T12:28:18Z

aeron-driver/src/main/java/io/aeron/driver/DriverConductor.java

+    private static String congestionControlName(final CongestionControl congestionControl)
+    {
+        if (congestionControl instanceof StaticWindowCongestionControl)
+        {
+            return "static";
+        }
+        else if (congestionControl instanceof CubicCongestionControl)
+        {
+            return "cubic";
+        }
+        else
+        {
+            return congestionControl.getClass().getName();
+        }
+    }


Let's use congestionControl.getClass().getName() for simplicity. And it also aligns with the flow control handling.

Ok. Will fix.

pveentjer · 2024-12-20T05:25:24Z

Closing this PR since it breaks compatibility.

pveentjer requested a review from vyazelenko December 12, 2024 08:42

vyazelenko requested changes Dec 12, 2024

View reviewed changes

vyazelenko reviewed Dec 12, 2024

View reviewed changes

pveentjer force-pushed the feature/logbuffer-channel-uri-take2 branch 2 times, most recently from 88591aa to c8fc3df Compare December 16, 2024 05:05

vyazelenko reviewed Dec 16, 2024

View reviewed changes

vyazelenko requested changes Dec 16, 2024

View reviewed changes

mikeb01 reviewed Dec 16, 2024

View reviewed changes

aeron-client/src/main/java/io/aeron/ChannelUriStringBuilder.java Outdated Show resolved Hide resolved

vyazelenko requested changes Dec 17, 2024

View reviewed changes

pveentjer added 5 commits December 17, 2024 14:33

Initial work

237ca11

More params

ef302a8

More work on enrichImagePublicationChannelUri

2cf03ad

Processed review comments

6aa45a4

Processed review comments

94a4b3d

pveentjer force-pushed the feature/logbuffer-channel-uri-take2 branch from 0d3f190 to 94a4b3d Compare December 17, 2024 12:42

pveentjer added 3 commits December 17, 2024 16:30

Fixed broken test

c035f75

Fixes a memory ordering problem with channel uri.

15c918e

Minor doc improvement.

d8fe9cf

pveentjer closed this Dec 20, 2024

pveentjer mentioned this pull request Dec 20, 2024

Logbuffer metadata extra fields #1700

Merged


		LOG_META_DATA_LENGTH = align(LOG_CHANNEL_URI_OFFSET + SIZE_OF_INT + LOG_CHANNEL_URI_MAX_LENGTH, PAGE_MIN_SIZE);

		System.out.println("offset:"+offset);

		final String ccStr = udpChannel.channelUri().get(CONGESTION_CONTROL_PARAM_NAME);
		uriStringBuilder.congestionControl(null == ccStr ? "static" : ccStr);

		System.out.println("Set channelUri:" + channelUri(logMetaData));
		}

		configureUriStringBuilderWithPublicationParams(params, uriStringBuilder);


		uriStringBuilder.streamId(streamId);

		private Integer streamId;
		private Integer publicationWindowLength;


		channelUriStringBuilder.receiverWindowLength(params.receiverWindowLength);

		// channelUriStringBuilder.group(params.group);

[WIP] LogBuffer metadata store channelUri #1697

[WIP] LogBuffer metadata store channelUri #1697

Conversation

pveentjer commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pveentjer commented Dec 20, 2024