Skip to content

[FLINK-9841] Web UI only show partial taskmanager log#6329

Closed
yanghua wants to merge 1 commit intoapache:masterfrom
yanghua:FLINK-9841
Closed

[FLINK-9841] Web UI only show partial taskmanager log#6329
yanghua wants to merge 1 commit intoapache:masterfrom
yanghua:FLINK-9841

Conversation

@yanghua
Copy link
Copy Markdown
Contributor

@yanghua yanghua commented Jul 13, 2018

What is the purpose of the change

This pull request fixed a bug triggered web UI only show partial taskmanager log

Brief change log

  • Remove the redundant resource close

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

Copy link
Copy Markdown
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

I doubt that the "double close" is causing it, a more likely scenario is that we're closing the file before it was completely written; after all Channel#writeAndFlush isn't blocking.

@yanghua
Copy link
Copy Markdown
Contributor Author

yanghua commented Jul 13, 2018

@zentol yes, you are right, sorry about my expression. here we should not use try-with-resource, because the listener will close the file. And it seems try-with-resource closes faster than the complete listener.

@yanghua
Copy link
Copy Markdown
Contributor Author

yanghua commented Jul 15, 2018

@dawidwys can you review this PR?

// HttpChunkedInput will write the end marker (LastHttpContent) for us.
}
try {
fileLength = randomAccessFile.length();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you could move this back into the following try block, like this:

try {
			final long fileLength = randomAccessFile.length();
			final FileChannel fileChannel = randomAccessFile.getChannel();

			try {
				[... actual handler code ...]
			} catch (Exception e) {
				fileChannel.close();
				throw e;
			}
		} catch (IOException ioe) {
			try {
				randomAccessFile.close();
			} catch (IOException e) {
				log.warn("Error while closing file.", e);
			}
		}

@yanghua
Copy link
Copy Markdown
Contributor Author

yanghua commented Jul 18, 2018

@zentol this PR match your requirement? I hope it can be merged into 1.6, so that users can see the full taskmanager log.

zentol pushed a commit to zentol/flink that referenced this pull request Jul 23, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 23, 2018
@asfgit asfgit closed this in 8d58bf4 Jul 23, 2018
sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018
@jerry-024
Copy link
Copy Markdown
Contributor

@zentol yes, you are right, sorry about my expression. here we should not use try-with-resource, because the listener will close the file. And it seems try-with-resource closes faster than the complete listener.

hi @yanghua do you have test case for this solution.

@yanghua
Copy link
Copy Markdown
Contributor Author

yanghua commented Dec 13, 2018

@jinglining Not currently, I encountered this problem in our production environment, and I fixed it and verified it. It has been merged into Flink 1.6.0, and our current production environment version is 1.6.0. It behaves normally in our environment. What problem have you encountered?

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.

4 participants